You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This PR gives only one chance. As soon as it fails for any reason, we stop to listen (with writing failure to internal logs only once). It is still not resolving more interesting issue when WebSocketTransport class should behave more robust, but this is right improvement (and quick win).
🔄 Types of changes
Bug fix (backwards compatible)
PR Type
Bug fix
Description
Modify BiDi message receiving to fail fast on errors
Remove retry loop for failed WebSocket connections
Improve error handling with single attempt strategy
Diagram Walkthrough
flowchart LR
A["Receive BiDi Messages"] --> B["Single Attempt"]
B --> C["Success: Continue"]
B --> D["Failure: Log & Throw"]
D --> E["Stop Listening"]
Loading
File Walkthrough
Relevant files
Bug fix
Broker.cs
Implement fail-fast BiDi message reception
dotnet/src/webdriver/BiDi/Communication/Broker.cs
Restructured error handling in ReceiveMessagesAsync method
Removed infinite retry loop on WebSocket receive failures
Added fail-fast behavior with single error logging
Improved exception filtering to exclude OperationCanceledException
After the first non-cancellation exception, the method rethrows which may fault the receive task; verify upstream code handles this without leaving background tasks or event processing in an inconsistent state, and that reconnection (if desired elsewhere) is not required.
catch(Exceptionex)when(exis not OperationCanceledException){if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Couldn't process received BiDi remote message: {ex}");}throw;}
Error is now always logged once when enabled; confirm log level and message frequency meet expectations and that OperationCanceledException is indeed the only benign case to filter.
if(_logger.IsEnabled(LogEventLevel.Error)){_logger.Error($"Couldn't process received BiDi remote message: {ex}");}
Prevent unhandled exception from crashing application
In ReceiveMessagesAsync, handle exceptions by canceling the broker's operations via _cancellationTokenSource.Cancel() instead of re-throwing the exception, which would cause an unhandled exception and crash the application.
public async Task ConnectAsync(CancellationToken cancellationToken)
{
await _transport.ConnectAsync(_url, cancellationToken).ConfigureAwait(false);
_ = ReceiveMessagesAsync(cancellationToken);
_ = ProcessEventsAwaiterAsync();
}
private async Task ReceiveMessagesAsync(CancellationToken cancellationToken)
{
try
{
while (!cancellationToken.IsCancellationRequested)
{
var data = await _transport.ReceiveAsync(cancellationToken).ConfigureAwait(false);
ProcessReceivedMessage(data);
}
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
if (_logger.IsEnabled(LogEventLevel.Error))
{
_logger.Error($"Couldn't process received BiDi remote message: {ex}");
}
- throw;+ // A reception error is fatal for the connection.+ // Trigger cancellation for all broker operations to shut down gracefully.+ _cancellationTokenSource.Cancel();
}
}
[To ensure code accuracy, apply this suggestion manually]
Suggestion importance[1-10]: 10
__
Why: The suggestion correctly identifies a critical issue introduced by the PR where an unobserved exception from a "fire-and-forget" task would crash the application, and it proposes a robust solution.
This file contains hidden or 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
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.
User description
BiDi Broker listens to incoming WebSocket messages and repeats even if remote end is died.
🔗 Related Issues
Fixes #16134
💥 What does this PR do?
This PR gives only one chance. As soon as it fails for any reason, we stop to listen (with writing failure to internal logs only once). It is still not resolving more interesting issue when
WebSocketTransportclass should behave more robust, but this is right improvement (and quick win).🔄 Types of changes
PR Type
Bug fix
Description
Modify BiDi message receiving to fail fast on errors
Remove retry loop for failed WebSocket connections
Improve error handling with single attempt strategy
Diagram Walkthrough
File Walkthrough
Broker.cs
Implement fail-fast BiDi message receptiondotnet/src/webdriver/BiDi/Communication/Broker.cs
ReceiveMessagesAsyncmethodOperationCanceledException