Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NioPipelineParser: possible message ordering issue in Dispatch? #189

Open
TristanPerry opened this issue Mar 19, 2019 · 3 comments
Open

Comments

@TristanPerry
Copy link

Issue:

From looking at NioPipelineParser, I'm wondering whether messages for a specific callId could be processed out of order?

Reason:

#59 / 6e5a46f reworked the sipStack's selfRoutingThreadpoolExecutor to use a ThreadAffinityExecutor which will re-use the same thread executor (from the pool) if the ThreadAffinityIdentifier.getThreadHash() for a submitted object is not null.

#44 / 17743fe reworked the NioPipelineParser to remove the semaphore+queue based solution (for message ordering) with the aim to instead use:

A simple thread pool with thread affinity by call id

I might be wrong, although I don't think this is actually happening within the pipeline? I say this because Dispatch does not implement ThreadAffinityIdentifier and thus ThreadAffinityExecutor.calculateAffinityThread() will just retrieve the next thread (instead of scheduling with the same executor)?

Potential fix:

Change the Dispatch method to be:

public class Dispatch implements Runnable, QueuedMessageDispatchBase, ThreadAffinityIdentifier {
    	String callId;
        UnparsedMessage unparsedMessage;
    	long time;

    	public Dispatch(UnparsedMessage unparsedMsg, String callId) {
    		this.unparsedMessage = unparsedMsg;
    		this.callId = callId;
    		time = System.currentTimeMillis();
    	}

        @Override
        public Object getThreadHash() {
            return this.callId;
        }

        ...
@dinoopp
Copy link

dinoopp commented Sep 29, 2021

@TristanPerry Yes this issue exists! Did you get any update/fix for this?

@TristanPerry
Copy link
Author

Unfortunately I didn't get any update @dinoopp , although the potential fix that I mention (i.e. ensuring that callId is saved in the class, then overriding the getThreadHash() method) seemed to work fine for us.

@dinoopp
Copy link

dinoopp commented Sep 29, 2021

@TristanPerry thats great to know that it is working. Is your product still using this stack and have you done any further improvements?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants