-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
feat(websockets): add disconnect reason parameter to OnGatewayDisconnect #15451
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build ccc32411-c048-48b5-946a-1ddf86b11307Details
💛 - Coveralls |
This change enhances the WebSocket disconnect handling by providing the disconnect reason as an optional second parameter to the handleDisconnect method. Changes: - Add optional reason parameter to OnGatewayDisconnect interface - Update NestGateway interface to support disconnect reason - Modify WebSocketsController to capture and forward disconnect reason - Enhance IoAdapter to extract reason from Socket.IO disconnect events - Maintain full backward compatibility with existing implementations - Add comprehensive unit and integration tests The disconnect reason helps developers understand why clients disconnect, enabling better error handling and debugging. Common reasons include 'client namespace disconnect', 'transport close', 'ping timeout', etc. This change is fully backward compatible - existing code continues to work without modification while new code can optionally access the disconnect reason. Closes nestjs#15437 Signed-off-by: snowykte0426 <[email protected]>
const filters = this.filters.filter( | ||
filter => filter.exceptionMetatypes?.length === 0, | ||
); | ||
if (filters.length > 0) { | ||
return filters[0].func(exception, host); | ||
} |
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.
why(?)
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.
This change prioritizes global exception filters (i.e., filters with empty exceptionMetatypes) over more specific ones. Previously, global filters that were intended to catch all exceptions could be skipped because they were processed after specific filters.
The updated logic ensures that if a filter is designed to handle all exception types (as indicated by an empty exceptionMetatypes array), it is invoked first, preserving the intended filter hierarchy and behavior.
Please let me know if I’ve misunderstood any part of this change.
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.
Previously, global filters that were intended to catch all exceptions could be skipped because they were processed after specific filters.
That's the expected behavior.
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.
Thanks for the clarification! I've removed the exception filter changes
in commit 6cf7ad5 since the current behavior is indeed the intended
behavior.
event | ||
.pipe(distinctUntilChanged()) | ||
.subscribe(instance.handleDisconnect.bind(instance)); | ||
event.pipe(distinctUntilChanged()).subscribe((data: any) => { |
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.
dont think distinctUntilChanged
will work as expected now
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.
You’re right — thank you for pointing that out.
Since we’re now emitting objects like { client, reason }
instead of just the client, distinctUntilChanged()
was comparing object references, which caused the deduplication to fail.
I’ve updated the code to include a custom comparison function for distinctUntilChanged()
. This handles both the previous format (just client) and the new one ({ client, reason }
) by extracting and comparing the actual client values.
Really appreciate you catching this.
Fix the distinctUntilChanged operator in subscribeDisconnectEvent to properly compare client objects when using the new { client, reason } format. The previous implementation would not deduplicate correctly as it compared object references instead of the actual client instances. This ensures backward compatibility while properly handling both the old format (just client) and new format ({ client, reason }) for disconnect events. Signed-off-by: snowykte0426 <[email protected]>
Remove the changes to exception filter handling in RPC exceptions handler as the current behavior is the intended behavior according to maintainer feedback. Signed-off-by: snowykte0426 <[email protected]>
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Currently, when a WebSocket client disconnects, the handleDisconnect
method only receives the client object without any information about why
the disconnection occurred. This makes it difficult for developers to
implement different handling logic based on the disconnect reason (e.g.,
client-initiated vs. network error vs. timeout).
Issue Number: #15437
What is the new behavior?
This PR enhances the WebSocket disconnect handling by providing the
disconnect reason as an optional second parameter to the handleDisconnect
method.
Changes:
OnGatewayDisconnect
Interface: Added optional reason?: string parameterto handleDisconnect method
NestGateway
Interface: Updated to support the new disconnect reasonparameter
WebSocketsController
: Modified to capture and forward disconnect reasonfrom events
IoAdapter
: Enhanced to extract disconnect reason from Socket.IOdisconnect events
Usage:
After (new feature):
Common Disconnect Reasons:
'client namespace disconnect'
- Client explicitly called disconnect()'server namespace disconnect'
- Server forced disconnection'transport close'
- Underlying transport was closed'transport error'
- Transport encountered an error'ping timeout'
- Client didn't respond to ping in timeDoes this PR introduce a breaking change?
Other information
Implementation Details:
The implementation modifies the following core components:
now support the optional reason parameter
WebSocketsController → Gateway
handles both old and new formats seamlessly
properly handle both old format (just client) and new format (
{ client, reason }
) for disconnect events