Update_the_default_SubscriberDeliveryTaskCount

[MENNAELMESALMY]

This pull request is meant to solve this issue #450.
The added test was tried against different numbers of SubscriberDeliveryTaskCount, it takes around ~500 ms to process, leaving it to 0 (as was the default before) takes more than 1000 ms ( ~7000ms).

[scottf]

There are 2 reasons that #450 has been around so long and probably should have been closed.

1. Making this change would actually not be backward compatible.
2. The user can change this since it's an option.

I think this functionality would actually be best served by having one an full example and adding documentation, not by changing the default.

[MENNAELMESALMY]

Hi @scottf
Can you tell me more what should be best demonstrated in the documentation example? I believe this is the current documentation and is generated from the comments in the code?
Also if it will be incompatible in some cases we could add a warning in the documentation so no one will set it to that value by mistake, can you provide more details on that?

[scottf]

So by documentation I mean a full sample and probably stuff in the readme. We know the project could certainly use more docs.
My concern about backward compatibility is that the default behavior would change, but we could document this. I've reached out and asked someone else if they think this would be okay, I should hear back tomorrow.

[ColinSullivan1]

Thanks for the contribution! I think the new default (1) is reasonable. This change would affect users who have a small number of subscribers and require extremely high throughput. This change should certainly be called out in release notes.

Minor fix in the doc, otherwise LGTM! Thanks!

[ColinSullivan1]

LGTM. Thanks!

[scottf]

@MENNA123MAHMOUD Can you please look at IntegrationTests.TestSubscriptions.TestAsyncSubscribersOnClose
This test is failing with the change. When I put the option back to zero, the test passes.

[MENNAELMESALMY]

Hi @scottf
I looked into IntegrationTests.TestSubscriptions.TestAsyncSubscribersOnClose and it was failing with A task may only be disposed if it is in a completion state (RanToCompletion, Faulted or Canceled). .

            internal Channel<Msg> getChannel()
            {
                // simple round robin, adding Channels/Tasks as necessary.
                lock (pLock)
                {
                    if (current == maxTasks)
                        current = 0;

                    if (pList.Count == current)
                        pList.Add(new SubChannelProcessor(connection));

                    return pList[current++].Channel;
                }
            }

The SubChannelProcessor class was creating Tasks with no cancellation token so when the channel was disposed tasks was still not marked as completed hence the error. Sending a token and signaling with it the other tasks to cancel solves the issue.

[MENNAELMESALMY]

However now this test IntegrationTests.TestBasic.TestUrlArgument is failing due to timeout connecting to Nats, it works fine locally on my laptop though and doesn't seem related to my change as it doesn't create any async subscribers for example.
Any ideas on how to approach this @scottf @ColinSullivan1

[scottf]

@MENNA123MAHMOUD Don't worry about TestUrlArgument. Not sure why it fails intermittently, I've been looking into it, but like you , it works fine on my machine.

[MENNAELMESALMY]

Hi @ColinSullivan1 @scottf
Is there something I could help with to get this merged?

[scottf]

@ColinSullivan1 I've been thinking about this a lot. I don't think we should change the default. The user can still change this because it's an option.
So remember multiple subscriptions are coming through the same channel. One behavior I noticed that delivery comes in bunches, which is simply how the server pushes messages to the client. But with multiple channels, this would all go in parallel. With a shared channel, one handler might get a bunch of messages, which they take time to handle and ack and that blocks the other handlers from getting their messages to work on.
This is not bad, but I think it should be a conscious choice to share the channel. This is what the java client does, give the developer to specify the "dispatcher" and whether subs share it.
At some point in the recent past, I actually started going down the dispatcher path for .net and would like to continue doing that.

[scottf]

@MENNA123MAHMOUD So there is one change that I definitely want and that's the change to SubChannelProcessor where you added the cancellation token. Would it be possible to make a PR just for that? This will allow us to get the improvement in and then continue considering how to address the rest of the problem.

[scottf]

Sorry one more note about sharing the channel. Since you setup a pool per connection, right now you would have to keep making connections to have different groups of subscribers in different channel pools.