Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

WebSockets can attempt to send when in the "reconnecting" state, resulting in an exception. #2582

Closed
NTaylorMullen opened this Issue · 7 comments

5 participants

@NTaylorMullen
Collaborator

Trying to send via the WebSocket transport when the connection is in the reconnecting state results in a thrown exception (other transports do not have this behavior).

In ShootR I'm running into invalid state errors; however, after looking through the code I'd expect to see null ref exception because the socket should be null. Either way the transport throws when trying to send when other transports do not.

The other transports create new ajax requests which is why they do not throw when in the reconnecting state, where the WebSocket transport relies on the socket being active. We should detect this scenario prior to attempting a send and trigger a connection error instead of raising the event to window.error.

@DamianEdwards

I'm assuming this is the JS client. What is the exception exactly? Don't we expect websockets to throw if you try to send while reconnecting? We've always know it behaved different to the other transports in this regard but it shouldn't be an issue if the application is correctly tracking the connection state by handling the state changed event, yes?

@NTaylorMullen
Collaborator

Yup JS client, and the exception I ran into was an invalid state error. Essentially the client was trying to send via the WebSocket object prior to the connection being reopened.

If the application is tracking the connection state this is not a problem, correct. However, this error does not occur with any other transport. I agree that the behavior should be different but I don't think we should be allowing a top level error to be thrown.

@signalrcoreteam
Collaborator

Let's change websockets.send to catch send errors, raise connection.error and force the connection into reconnect.

@NTaylorMullen NTaylorMullen referenced this issue from a commit
@NTaylorMullen NTaylorMullen Added a try-catch to the WebSocket transport send.
- This helps catch errors that can be thrown from a websocket being in an invalid state.
- Also added a test to verify that we catch errors thrown by a socket send.

#2582
0e82bd2
@halter73 halter73 was assigned
@NTaylorMullen NTaylorMullen referenced this issue from a commit
@NTaylorMullen NTaylorMullen Added a try-catch to the WebSocket transport send.
- This helps catch errors that can be thrown from a websocket being in an invalid state.
- Also added a test to verify that we catch errors thrown by a socket send.

#2582
b3e7452
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure the fail handler on hub invocations gets triggered
- Previously if you invoked a hub method while the WebSocket transport
  was in in the reconnecting state, it would throw.

#2582
ebcef8e
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure Connection.Send doesn't silently fail due to a reconnecting WS
- .NET Client
- Previously WebSocketTransport.Send would noop if the WebSocket wasn't
  open, and this could occur while in the reconnecting state.

#2582
5e0b865
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure Connection.Send doesn't silently fail due to a reconnecting WS
- .NET Client
- Previously WebSocketTransport.Send would noop if the WebSocket wasn't
  open, and this could occur while in the reconnecting state.

#2582
f763d3e
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure Connection.Send doesn't silently fail due to a reconnecting WS
- .NET Client
- Previously WebSocketTransport.Send would noop if the WebSocket wasn't
  open, and this could occur while in the reconnecting state.

#2582
a6056c9
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure Connection.Send doesn't silently fail due to a reconnecting WS
- .NET Client
- Previously WebSocketTransport.Send would noop if the WebSocket wasn't
  open, and this could occur while in the reconnecting state.

#2582
9efc90b
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure Connection.Send doesn't silently fail due to a reconnecting WS
- .NET Client
- Previously WebSocketTransport.Send would noop if the WebSocket wasn't
  open, and this could occur while in the reconnecting state.

#2582
ab36198
@NTaylorMullen NTaylorMullen referenced this issue from a commit
@NTaylorMullen NTaylorMullen Added a try-catch to the WebSocket transport send.
- This helps catch errors that can be thrown from a websocket being in an invalid state.
- Also added a test to verify that we catch errors thrown by a socket send.

#2582
cb550e1
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure the fail handler on hub invocations gets triggered
- Previously if you invoked a hub method while the WebSocket transport
  was in in the reconnecting state, it would throw.

#2582
004ead4
@NTaylorMullen NTaylorMullen referenced this issue from a commit
@NTaylorMullen NTaylorMullen Added a try-catch to the WebSocket transport send.
- This helps catch errors that can be thrown from a websocket being in an invalid state.
- Also added a test to verify that we catch errors thrown by a socket send.

#2582
2a0d805
@halter73 halter73 referenced this issue from a commit
@halter73 halter73 Ensure the fail handler on hub invocations gets triggered
- Previously if you invoked a hub method while the WebSocket transport
  was in in the reconnecting state, it would throw.

#2582
0e7de42
@gustavo-armenta

@halter73 @NTaylorMullen ,
I have looked at your commits and I have some comments:
1. C# client checks state and throws before attempting to send. I would have expected a sendAsync.ContinueWith to handle the error after it actually fails to send... that's done correctly on the JS client
2. JS client assumes send fails due to invalid state, but error message does not include current state. I think a send could fail also due to a large message and in this case we are misleading.

@halter73 halter73 was assigned
@halter73
Collaborator

@gustavo-armenta
1. The C# client checks the WebSocket state, not the SignalR connection state, so I think it is better to fail immediately. If we didn't check state beforehand, the underlying call to the WebSocketHandler.SendAsync would simply no-op. This is unobservable, unlike with the JS client where calling WebSocket.send while in a bad state will throw. Additionally, any ContinueWith callback will still be triggered with a faulted task.
2. I'm pretty sure we kill the socket if the message is too large, thereby putting the socket in an invalid state making the error message correct. The SignalR state will always become "reconnecting" at this point. I'll verify this is actually the case though.

@halter73
Collaborator

@gustavo-armenta I looked further into your second point, and I have verified that the behavior of both the C# and JS client remain unchanged when the server closes the WS connection due to an overly large message from the client.

The server will "clean"ly close the socket in this circumstance causing the clients to automatically reconnect. This will cause any active hub invocations to fail with the following error: "Connection started reconnecting before invocation result was received."

Any subsequent sends or hub invocations occurring before the reconnect completes will trigger the connection error handler (JS) or return a faulted task (C#). That, however, is the new intended behavior.

@gustavo-armenta

verified sending a message during reconnect raises connection.error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.