RestComm / jain-sip

Disclaimer: This repository is a git-svn mirror of the project found at http://java.net/projects/jsip whose original repository is developed collaboratively by the Advanced Networking Technologies Division at the National Institute of Standards and Technology (NIST) - an agency of the United States Department of Commerce and by a community of individual and enterprise contributors. TeleStax, Inc. will perform some productization work, new features experimentation branches, etc for its TelScale jSIP product that doesn't concern the community from the main repository hence this git repository.
http://www.restcomm.com/
144 stars 152 forks source link

NioPipelineParser: possible message ordering issue in Dispatch? #189

Open TristanPerry opened 5 years ago

TristanPerry commented 5 years ago

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 commented 3 years ago

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

TristanPerry commented 3 years ago

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 commented 3 years ago

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