Fix #276 Streaming subscription throws connection error - #297

Hi Avrom (@avromf), I'm your friendly neighborhood Azure Pull Request Bot (You can call me AZPRBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.azure.com.

TTYL, AZPRBOT;

Avrom (@avromf), Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, AZPRBOT;

Avrom (@avromf) can you provide more information on the incident? Are there any existing issues corresponding to this (if not will you create one)?

Yes, it is in the title, issue #276.

In a nutshell, my fix here follows Mike Noordermeer (@MikeN123) 's suggestion in the comments on issue #276 that a separate pooling manager should be created for subscriptions.

Please note that the PoolingHttpClientConnectionManager only creates 2 connections per route by default, so you can only have 2 subscriptions at the moment (I think). Maybe that should be fixed, other than that it seems like an ok workaround to me (but I did not look at the PR in detail).

A better solution would be to rewrite the complete connection handling, but that's would probably be quite some work.

Mike Noordermeer (@MikeN123) I went back and tested this by creating more than 2 subscriptions and you are correct. It hits the default limit of 2.
I also went back and took a look at the original code before the change to BasicHttpClientConnectionManager. It also had the same limitation.
Should we increase the limit and if so, what should we increase it to?

10 would probably be ok, with an option to change it, since that is also the default value for Exchange (HangingConnectionLimit setting)

Avrom (@avromf) are you planning any further changes on this?

just so you guys know I just verified this appears to have fixed this problem for us

i'm happy to wrap this up if Avrom (@avromf) is unable to at this time (he did all the hard work already). is the only concern the connection pool?

I'm currently busy today and tomorrow, but I may be able to take care of it next week.
Setting the connection pool to increase the limit beyond 2 is not an issue, my only concern is Mike Noordermeer (@MikeN123) suggestion to have an option to change it. How is that supposed to be done? Via a method call?

A method call should be ok (and maybe an overloaded constructor for convenience?). I would also increase the defaults to 10 or something. Docs on how to set the limits can be found here: https://hc.apache.org/httpcomponents-client-4.4.x/tutorial/html/connmgmt.html#d5e393

Even though a number of 10 seems some kind of random to me this should be more usefull then the restricted value of 2. Once added we should also include an update to the readme/wiki to mention the possible change of the connection size.

10 was not really randomly chosen, it's the default max concurrency of EWS. But the streaming subscriptions seem to use another limit (HangingConnectionLimit), which seems to have a default of 3 according to https://msdn.microsoft.com/en-us/library/office/jj945066(v=exchg.150).aspx#bk_ThrottlingNotifications

Mike Noordermeer (@MikeN123) The only thing I was wondering about is that it was also suggested that the implementation be completely written. Assuming this is actually done some time in the future, that may possibly make any API changes (like the addition of a method to set the number of connections in the pool) obsolete.
Should we then simply leave it at max 10 since the chances that someone may need to change it are rare?

It's probably a small effort to add 1 method that gives the opportunity to change the maximum, so why not add it then?

Avrom (@avromf) any update on this? Are there any questions left or are you just busy?

No, no more questions, just currently busy.

Avrom (@avromf) maybe we should get this in shortly. If you dont have time Kenton Gray (@kentongray) should support as offered.

Avrom (@avromf) this PR is now open for over 1 month. I would love to have this integrated in stable 2.0 release. So if you want your name on this change please have this PR updated and apply the wanted changes. If I dont notice any changes here within the next days I will address this issue on my own and close this PR due to lag of contribution.

Sorry for the delay, André (@serious6) . I've now set the default number of connections for the pooling manager to 10 and added a method allowing one to change it if needed.
I've also merged it with the latest. (I hope I did that correctly.)

Avrom (@avromf) thanks for the work. Unfortunately your commits also include some conflict resolution which we should not have on the upstream/master.Maybe you can reset these commits resolve the conflicts and only commit the changes made? If you are facing any trouble plz report back.

While in my attempt to fix this, this pull request ended up being closed thus I have created a new one.
#336