-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Java] Fix skip negotiate null ref #30482
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge. |
| TransportEnum chosenTransport; | ||
| if (this.skipNegotiate) { | ||
| if (this.transportEnum != TransportEnum.WEBSOCKETS) { | ||
| throw new RuntimeException("Negotiation can only be skipped when using the WebSocket transport directly."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could make it a bit more clear in the doc comments that the transport must explicitly be set to {@link TransportEnum.WEBSOCKETS} rather than it being something that happens automatically when you call .shouldSkipNegotiate(true).
Lines 70 to 77 in c925f99
| /** | |
| * Indicates to the {@link HubConnection} that it should skip the negotiate process. | |
| * Note: This option only works with the Websockets transport and the Azure SignalR Service require the negotiate step. | |
| * | |
| * @param skipNegotiate Boolean indicating if the {@link HubConnection} should skip the negotiate step. | |
| * @return This instance of the HttpHubConnectionBuilder. | |
| */ | |
| public HttpHubConnectionBuilder shouldSkipNegotiate(boolean skipNegotiate) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let this ruminate over the weekend, and I'm going to remove this check from the 5.0 change and add it to the main branch.
This would break folks who didn't set a transport and it would use WebSockets first by default but now would throw.
Description
Using
.shouldSkipNegotiate(true)with the SignalR Java client will null ref.Customer Impact
Can't use the skip negotiate feature, useful for environments where you can't/wont enable sticky sessions, and/or always know WebSockets will be used.
Regression?
Regressed from 3.1
Risk
The issue is very obvious, the reason it wasn't found during the 5.0 release is because of an unfortunate piece of the testing that hid the issue. Fixed by writing a new test.
Verification
Packaging changes reviewed?
Addresses #29765