[release/8.0] Handle rejected promises inside incoming Invocation messages #55230
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Incoming messages of Invocation type result in executing a clientMethod promise, but promise rejections are unhandled.
This can result in a unhandled rejected promise, potentially resulting in Node.js process crashing.
Note, this was previously detected via eslint and suppressed in code.
To avoid this, catch promise rejections and log the failure.
One scenario this case can happen is, if during the clientMethod call, the server connection is disconnected, the invokation will still attempt to send a completion message (through _sendWithProtocol). This will then result in a promise rejection, such as the following:
```
Cannot send data if the connection is not in the 'Connected' State.
at HttpConnection.send (node_modules\@microsoft\signalr\dist\cjs\HttpConnection.js:95:35)
at HubConnection._sendMessage (node_modules\@microsoft\signalr\dist\cjs\HubConnection.js:266:32)
at HubConnection._sendWithProtocol (node_modules\@microsoft\signalr\dist\cjs\HubConnection.js:273:21)
at HubConnection._invokeClientMethod (node_modules\@microsoft\signalr\dist\cjs\HubConnection.js:577:24)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
```
Adds a test covering when callback invoked when server invokes a method on the client and then handles rejected promise on send
When the unhandled promise fix is not applied to HubConnection,
the following error fails the HubConnection.test.ts test suite:
Send error
831 | connection.send = () => {
832 | promiseRejected = true;
> 833 | return Promise.reject(new Error("Send error"));
| ^
834 | }
835 | p.resolve();
836 | });
at TestConnection.connection.send (signalr/tests/HubConnection.test.ts:833:51)
at HubConnection.send [as _sendMessage] (signalr/src/HubConnection.ts:422:32)
at HubConnection._sendMessage [as _sendWithProtocol] (signalr/src/HubConnection.ts:433:25)
at HubConnection._sendWithProtocol [as _invokeClientMethod] (signalr/src/HubConnection.ts:808:24)
github-actions
bot
requested review from
BrennanConroy and
halter73
as code owners
dotnet-issue-labeler
bot
added
the
area-signalr
BrennanConroy
added
Servicing-consider
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
dotnet-policy-service
bot
added
the
pending-ci-rerun
wtgodbe
approved these changes
May 2, 2024
wtgodbe
removed
the
pending-ci-rerun
|
@BrennanConroy @halter73 good to go? |
|
Stephen and I approved the PR on main, I assume his approval won't change for release/8.0 😄 . So let's merge! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #52523 to release/8.0
/cc @BrennanConroy @damirault
Handle rejected promises inside incoming Invocation messages
Description
Using client results, which is a new feature in 7.0, the client sends a response back to the server after producing some sort of result. When sending the response back it's possible the connection closed, the SignalR library doesn't properly handle this and will result in an unhandled rejected promise.
Fixes #52524
Customer Impact
This can result in an unhandled rejected promise, which is an unwanted log in the browser and can crash the process if running in Node.js.
Regression?
Comes from a new feature introduced in 7.0 and has been there since.
Risk
Small, easy to understand fix. Plus a nice test case to make sure it stays fixed.
Verification
Packaging changes reviewed?