InvalidOperationException when set HubProxy Item property during executing HubProxy Invoke method in SignalR .Net client

[tanaka-takayoshi]

Functional impact

I found InvalidOperationException is thrown with HubProxy _state lock condition.
I use latest version of Microsoft.AspNet.SignalR.Client (2.2.0).

Repro steps

1. Create HubProxy
2. Execute below 2 method in parrarel
   1. set HubProxy Item property
   2. execute HubProxy Invoke method

Expected result

Invoke method runs with successfully.

Actual result

Then Invoke method may thorw InvalidOperationException with "Collection was modified; enumeration operation may not execute.".

Further technical details

HubProxy Item Property (indexer) gets or sets the "_state" field with lock statement.
https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Client/Hubs/HubProxy.cs#L27-L45

The "_state" is IDictionary<string, JToken> and this filed passes to "hubData" local variable in Invoke method.
https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Client/Hubs/HubProxy.cs#L159-L172

So InvalidOperationException is thrown if Item property is set during executing Invoke method.
This is because dictionary in "hubData" variable is enumerated in Json.Net library.
https://github.com/JamesNK/Newtonsoft.Json/blob/master/Src/Newtonsoft.Json/Serialization/JsonSerializerInternalWriter.cs#L926

Just an idea, this code will solve this issue.

lock(_state)
{
    if (_state.Count != 0)
    {
        hubData.State = _state;
    }

    var value = _connection.JsonSerializeObject(hubData);
}

[moozzyk]

Why do you want to set the state and call Invoke in parallel? The state is sent to the server when you call Invoke so these calls should be serialized otherwise there is a race condition and the state data you set may not to be sent to the server. Also the indexer is a synchronous method so setting it "in parallel with Invoke" means that either you create your own threads in which case you should ensure the right ordering of calls or you first invoke the Invoke method without awaiting and then you set the state in which case you probably should reorder the calls.
Can you elaborate more on your scenario?

[tanaka-takayoshi]

I found this issue during I wrote a stress test tool for SignalR WebServer. Several worker threads run in parallel and these threads share the same HubProxy instance.
I understand my case is a special case. But HubProxy should be a thread safe class. Actually, _state filed is locked during Item Property (indexer). Invoke method should lock _state field in the same manner.

[goldenc]

I think this is the same issue as the one i raised here: #3631

I agree with tanaka, there is inconsistent locking around _state. Either its thread safe or its not, the existence of locking at all suggests to me that it should be thread safe.

[dunkymole]

Almost a year old an no comment :-(

[daniel-smith]

The OP mentions that parallel calls to the indexer and Invoke expose this bug. However, it seems that two parallel calls to Invoke will also expose the issue, as the callback sets state via that same indexer. https://github.com/SignalR/SignalR/blob/master/src/Microsoft.AspNet.SignalR.Client/Hubs/HubProxy.cs#L128
The callback may be setting state, while another thread's call to Invoke is serialising (and therefore enumerating) the state. I'm seeing this issue in some automated tests that new up a proxy and call various hub methods in parallel. It is a real-world scenario to call hub methods in parallel, so this is a fairly serious bug in my view.

[davidfowl]

Why do you need to call hub methods in parallel? Why not work around the issue but serializing calls to the hub proxy?

[tanaka-takayoshi]

Actually I can work around. However, as goldenc and daniel-smith said, there is inconsistent locking.
In particular, I’m seeing this issue in my own load testing.
If we should call hubproxy in serial, it would be better to remove all lock statements around _state due to performance.
If we can call hubproxy in parrarel, it would be better to add lock statements around _state.

[davidfowl]

Agreed, I'm just making sure this isn't blocking you

[daniel-smith]

The reason we call things in parallel is simply because we await multiple, potentially long-ish running, request/response results in parallel. Even if we did serialise calls to Invoke, I would have thought we have no control over the callback thread - so I'm not sure this would solve the issue. I.e. we couldn't serialise the callback work, which can still update state while another serialized call to Invoke is happening.

[aspnet-hello]

This issue has been closed as part of issue clean-up as described in https://blogs.msdn.microsoft.com/webdev/2018/09/17/the-future-of-asp-net-signalr/. If you're still encountering this problem, please feel free to re-open and comment to let us know! We're still interested in hearing from you, the backlog just got a little big and we had to do a bulk clean up to get back on top of things. Thanks for your continued feedback!