Bug (NetworkManger): The key already existed in the dictionary

[Im5tu]

Recently we have been seeing the following under load:

ex_msg: The key already existed in the dictionary. | stacktrace: System.ArgumentException: The key already existed in the dictionary.
   at System.Collections.Concurrent.ConcurrentDictionary`2.System.Collections.Generic.IDictionary<TKey,TValue>.Add(TKey key, TValue value)
   at PuppeteerSharp.NetworkManager.<OnRequestWillBeSentAsync>d__44.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at PuppeteerSharp.NetworkManager.<Client_MessageReceived>d__36.MoveNext()

This PR looks at some of the causes for this. We are still testing the changes, but wanted to get this here for your visibility and thoughts also.

I've managed to:

• Remove two of the dictionaries that were previously required
• Optimise the way some of the dictionaries were used (ie: using TryRemove instead of Get/Remove)
• Fixed up some event invocation handling
• Fixed a few minor race conditions (ie: MultiMap.Add - note that there is still a problem when the list needs to grow the array, but from what I can see the default capacity should be enough to warrant not locking)

... all whilst hopefully kept the same semantics.

It'd be really nice if chrome fixed the mentioned bug and started to send back the request id, then the entire class could be handled with a single ConcurrentDictionary. I know this is not you though ;)

[kblok]

I'll go throw this tomorrow. But I get your point. Sometimes it sucks branching from Puppeteer's code but this makes sense.
Let's see what AppVeyor thinks about this :p

[Im5tu]

So it seems appveyor is having the same trouble I am running the test suite. Is this a known issue or more likely something that i've messed up?

[kblok]

@Im5tu can you pull from master?

[Im5tu]

@kblok - Done :)

[kblok]

WorkerTests.ShouldEmitCreatedAndDestroyedEvents is failing around here. request is null, so we fail setting RawRequest

[kblok]

I think we still have something on this test ShouldNotCrashWhileRedirectingIfOriginalRequest

[Im5tu]

It appears that we get the redirect response prior to getting the request will send portion. I haven't managed to take a look further yet - should be able to get back to resolving this tomorrow :)