Closed dsuresh-ap closed 3 years ago
Is there another way to dispose of a connection that is in the CONNECTING state?
2\. This happened to us because our server returns us 404 when attempting to connect with long polling so we `.close()` on it to stop it before switching to SSE.
The Java client doesn't have transport fallback currently, so a 404 should already close the connection.
at x.websockets.connection.core.SignalRCoreWebsocketConnection.disconnect(SignalRCoreWebsocketConnection.kt:185) at x.websockets.connection.core.OrchestratorCoreWebsocketConnection.disconnect(OrchestratorCoreWebsocketConnection.kt:65) at x.websocket.WebsocketManager.disconnectAllConnections(WebsocketManager.kt:116)
What is going on here? Did you replace the websocket connection? (And for my own curiosity how?)
If you are replacing the transport, you should make start
fail if the connections start results in a 404, and you shouldn't call .close
unless the connection has opened.
@BrennanConroy We have manual logic to switch from Websockets -> SignalR Core long polling or SSE (different lib).
If Websockets or long polling fails to connect or in our in-app debug transport switcher gets invoked, we call .stop()
on the current connection and null it out before attempting a new connection.
The NPE crash happened when we we called .stop()
on a connection that was in the CONNECTING
state.
So to be clear, we should:
Only call .close()
/.stop()
on a connection that is in the CONNECTED
state. Not on one that is in the DISCONNECTED/CONNECTING
state?
Please find our logic for stopping a connection and clearing out resources below.
override fun disconnect() {
shouldReconnect = false
connection?.let { connection ->
dataHandler.deregisterHubMethods(connection)
stopConnection(connection)
}
connectionDisposables.clear() // A composite disposable that contains all the disposables from any `.start()` or `.invoke()` subscriptions.
connection = null
}
private fun stopConnection(connection: HubConnection) =
try {
connection.stop().subscribeOn(Schedulers.io()).subscribe({
LogUtils.v(TAG, "Connection stopped")
}, {
LogUtils.w(TAG, "Connection stopped with error: ${it.localizedMessage}")
})
} catch (e: NullPointerException) {
// Due to an issue in the library we need to catch NPEs if we call connection.stop()
// when the connection is in the CONNECTING state
LogUtils.w(TAG, "Expected NPE when stopping connection: ${e.localizedMessage}")
}
Only call
.close()
/.stop()
on a connection that is in theCONNECTED
state. Not on one that is in theDISCONNECTED/CONNECTING
state?
Well, there is definitely a bug where we throw NPE, but normally if there was no bug this should be fine.
It's still unclear how/why you're calling stop. If the transport fails to connect, then the client will clean up and close the connection on its own.
Understood, thanks for the acknowledgement.
There are cases when we need to call stop()
on the connection. Ex:
onClosed
callback got triggered. From what you are saying we do not need to call stop()
in this case as if onClosed()
was called the connection already closed everything down internally? 1. User logged out and we need to close the connection before bringing them to login
That should be fine 99% of the time as the connection will be in the connected state.
2\. The connection needs to be closed before manually switching to another transport (long polling for SignalR Core or SSE using another library)
Still don't understand this. If WebSockets fails, the connection will close automatically.
3\. Feature flags that would disable SignalR Core for some users
Seems fine, if SignalR is disabled, you shouldn't have a connection to call stop on.
4\. Cleanup for sanity in the case `onClosed` callback got triggered. From what you are saying we do not need to call `stop()` in this case as if `onClosed()` was called the connection already closed everything down internally?
Correct.
For 1, 3 there is the slight chance that we are not connected or in a connecting state (poor internet, timing, etc). I get that this is a low chance but when we were running the 3.x version of this library our app got hit hard with a lot of crashes due to the server running SignalR having connection issues. This caused crashes in the app due to some other bugs that were fixed in 5.0. However to reproduce these crashes we use Charles to simulate weird connection issues (ex. denied requests) and was able to reproduce a crash like this. There was another crash in 5.0 that was fixed and when attempting to verify if that crash was fixed I was able to reproduce this crash.
Thanks for contacting us.
We're moving this issue to the Next sprint planning
milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
Hello. Just to inform me: Will this be fixed in a future version of the signarR library (5.0.4 for example) ?
Hi, I had the same issue today.
My use case is, I create a service associated with a heads-up notification.
Depending if the app is closed/open/background and if the device is locked/unlocked, the Android OS may open or not the activity associated with the heads-up notification.
If that activity opens, it will kill the service since it's not longer required.
In this use case, the service will be short live, and the SignalR connection may be stoped, before it's successfully connected to the server, and it will crash the app.
Do you have any plans o fixing this? Can I help to get this fixed? Thanks
Thanks for the reminder, opened a PR to fix this https://github.com/dotnet/aspnetcore/pull/34046
Describe the bug
If you call
close()
on a connection that did not establish yet (in the CONNECTING state) a NPE will be thrown due to the code callingtransport.stop()
on a null reference.See HubConnection.stop(String errorMessage)
To Reproduce
.start()
on a connection..close()
on it. This happened to us because our server returns us 404 when attempting to connect with long polling so we.close()
on it to stop it before switching to SSE.Exceptions (if any)co
Further technical details