Acquire HubConnectionStateLock before Send/Invoke/Stream #12078

mikaelm12 · 2019-07-11T01:15:42Z

Issue: #11995 , #12076

We need to acquire the hubConnectionStateLock before running the hubConnectionState checks in send/invoke/stream

We actually weren't checking the state at all when calling stream.

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java

mikaelm12 · 2019-07-11T22:46:58Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=261179&view=logs

aspnet-hello · 2019-07-11T22:47:42Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2771

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java

halter73 · 2019-07-12T22:02:07Z

I don't know if we want to do this as part of this PR, but we should really think about how we're using the hubConnectionStateLock in all the other HubConnection methods.

For example, it's important that HubConnection.start() acquires the lock at the beginning of the method when it checks whether it's in the DISCONNECTED state so that it cannot race with stop().

It's also incorrect for HubConnection.start() to later transition to the CONNECTED state after the handshake completes without at least verifying that you're still connecting and HubConnection.stop() hasn't been called. This probably requires a CONNECTING state.

The CONNECTING state would also help you know not to restart the transport. Right now, the second call to HubConnection.start() could actually start the transport again in parallel.

HubConnection.stop() should probably move the connection into a DISCONNECTING state. I get that it's too early to move to the DISCONNECTED state since HubConnection.stopConnection() hasn't been triggered by the transport yet, but we probably want to change states to ensure that when there's two consecutive calls to HubConnection.stop(), the second call no-ops.

Even simple calls like setBaseUrl() should lock since it checks that the state is disconnected. We wouldn't want allow the HubConnection to start before setBaseUrl() actually sets the baseUrl.

I'm fine with us creating separate issues for these.

halter73

Looks good so far. Make sure we file the follow up issue to look at locking in the other methods.

mikaelm12 · 2019-07-12T23:25:56Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=262972&view=logs

mikaelm12 · 2019-07-12T23:26:37Z

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=262973

aspnet-hello · 2019-07-12T23:26:52Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2809
https://github.com/aspnet/AspNetCore-Internal/issues/2711

aspnet-hello · 2019-07-12T23:31:49Z

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build. I've created/commented on the following issue(s)
https://github.com/aspnet/AspNetCore-Internal/issues/2358

- #12078

* Update WiX to signed build (#36865) - #12078 * Always download blazor-hotreload.js from app root (#36897) The middleware that we inject always listens to the root of the app. While this might not play very nicely if the app is running off a virtual path, listening to the root is what we do with the aspnet-browser-refresh.js script and we've had no issue reports with it thus far. Fixes #35555 * Improve Results.Problem and Results.ValidationProblem APIs (#36856) * [release/6.0-rc2] Update sdk to 6.0.100-rc.2.21470.55 (#36783) * Update sdk to 6.0.100-rc.2.21470.55 Co-authored-by: Will Godbe <[email protected]> Co-authored-by: Doug Bunting <[email protected]> Co-authored-by: Pranav K <[email protected]> Co-authored-by: Safia Abdalla <[email protected]> Co-authored-by: Stephen Halter <[email protected]> Co-authored-by: Will Godbe <[email protected]> Co-authored-by: Brennan <[email protected]>

* Update WiX to signed build - #12078 * Update wix to 1.0.0-v3.14.0.5722 Co-authored-by: Doug Bunting <[email protected]> Co-authored-by: Eric StJohn <[email protected]>

- dotnet#12078

* [release/6.0-rc2] Retarget DOTNETHOME when installing x64 on ARM64 (#36695) * Retarget DOTNETHOME when installing x64 on ARM64 * Make platform comparison case insenstive * Address feedback * Install x64 registry keys to different path on ARM64 machine Co-authored-by: Eric StJohn <[email protected]> * Update WiX to signed build (#36865) - #12078 * Update wix to 1.0.0-v3.14.0.5722 (#37329) Co-authored-by: Eric StJohn <[email protected]> * [release/6.0] Use WIX_NATIVE_MACHINE to detect native architecture of target machine (#37335) * Use WIX_NATIVE_MACHINE to detect native architecture of target machine * Reference WIX_NATIVE_MACHINE property Final bit to resolve what I'm tracking in #37290 Co-authored-by: Eric StJohn <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Doug Bunting <[email protected]>

mikaelm12 added 3 commits July 10, 2019 18:01

Locking and connection state check

58a32a7

clean up

31f50ae

clean up

a3235af

mikaelm12 requested review from analogrelay, BrennanConroy and halter73 as code owners July 11, 2019 01:15

BrennanConroy approved these changes Jul 11, 2019

View reviewed changes

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java Outdated Show resolved Hide resolved

Pilchie added the area-signalr Includes: SignalR clients and servers label Jul 11, 2019

jkotalik reviewed Jul 11, 2019

View reviewed changes

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java Show resolved Hide resolved

analogrelay added this to the 3.0.0-preview8 milestone Jul 11, 2019

moar locking

f05723f

BrennanConroy reviewed Jul 11, 2019

View reviewed changes

src/SignalR/clients/java/signalr/src/main/java/com/microsoft/signalr/HubConnection.java Outdated Show resolved Hide resolved

mikaelm12 added 3 commits July 11, 2019 16:03

clean up

8267ede

clean

629e122

lock everything

a8879a1

halter73 approved these changes Jul 12, 2019

View reviewed changes

mikaelm12 mentioned this pull request Jul 12, 2019

Add intermediate HubConnection states in Java client #12138

Open

mikaelm12 merged commit 8b9503e into master Jul 15, 2019

mikaelm12 deleted the mikaelm12/ConnectionStateLock branch July 15, 2019 18:31

This was referenced Jul 15, 2019

Check HubConnectionState before sending StreamInvocation #12076

Closed

ConnectionState.getNextInvocationId() Null pointer exception #11995

Closed

dougbu added a commit that referenced this pull request Sep 22, 2021

Update WiX to signed build

258b28d

- #12078

dougbu mentioned this pull request Sep 22, 2021

Update WiX to signed build #36865

Merged

dougbu added a commit that referenced this pull request Sep 23, 2021

Update WiX to signed build (#36865)

a96feda

- #12078

github-actions bot pushed a commit that referenced this pull request Oct 6, 2021

Update WiX to signed build - #12078

4274cdf

ghost pushed a commit that referenced this pull request Oct 6, 2021

[main] Update WiX to signed build (#37317)

4adf8f3

* Update WiX to signed build - #12078 * Update wix to 1.0.0-v3.14.0.5722 Co-authored-by: Doug Bunting <[email protected]> Co-authored-by: Eric StJohn <[email protected]>

ericstj pushed a commit to ericstj/aspnetcore that referenced this pull request Oct 8, 2021

Update WiX to signed build (dotnet#36865)

c381474

- dotnet#12078

ericstj pushed a commit to ericstj/aspnetcore that referenced this pull request Oct 8, 2021

Update WiX to signed build (dotnet#36865)

a394985

- dotnet#12078

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Acquire HubConnectionStateLock before Send/Invoke/Stream #12078

Acquire HubConnectionStateLock before Send/Invoke/Stream #12078

Uh oh!

mikaelm12 commented Jul 11, 2019

Uh oh!

Uh oh!

Uh oh!

mikaelm12 commented Jul 11, 2019

Uh oh!

aspnet-hello commented Jul 11, 2019

Uh oh!

Uh oh!

halter73 commented Jul 12, 2019

Uh oh!

halter73 left a comment

Uh oh!

mikaelm12 commented Jul 12, 2019

Uh oh!

mikaelm12 commented Jul 12, 2019

Uh oh!

aspnet-hello commented Jul 12, 2019

Uh oh!

aspnet-hello commented Jul 12, 2019

Uh oh!

Uh oh!

Acquire HubConnectionStateLock before Send/Invoke/Stream #12078

Acquire HubConnectionStateLock before Send/Invoke/Stream #12078

Uh oh!

Conversation

mikaelm12 commented Jul 11, 2019

Uh oh!

Uh oh!

Uh oh!

mikaelm12 commented Jul 11, 2019

Uh oh!

aspnet-hello commented Jul 11, 2019

Uh oh!

Uh oh!

halter73 commented Jul 12, 2019

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

mikaelm12 commented Jul 12, 2019

Uh oh!

mikaelm12 commented Jul 12, 2019

Uh oh!

aspnet-hello commented Jul 12, 2019

Uh oh!

aspnet-hello commented Jul 12, 2019

Uh oh!

Uh oh!