Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 51 additions & 0 deletions dotnet/src/webdriver/BiDi/EventStreamExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// <copyright file="EventStreamExtensions.cs" company="Selenium Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
// </copyright>

using System.Runtime.CompilerServices;

namespace OpenQA.Selenium.BiDi;

public static class EventStreamExtensions
{
Comment on lines +22 to +25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Public class missing xml summary 📘 Rule violation ✧ Quality

EventStreamExtensions is a new public class but it has no XML documentation comment with a
<summary> immediately preceding the declaration. This violates the requirement that all public API
members include <summary> docs for tooling and consumers.
Agent Prompt
## Issue description
A new public C# type (`EventStreamExtensions`) was added without an XML documentation comment containing a non-empty `<summary>`.

## Issue Context
Compliance requires XML docs with `<summary>` for all public API members.

## Fix Focus Areas
- dotnet/src/webdriver/BiDi/EventStreamExtensions.cs[22-25]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

/// <summary>
/// Configures how awaits on the tasks returned from an iteration of the event stream are performed.
/// </summary>
/// <remarks>
/// <para>
/// <see cref="IEventStream{TEventArgs}"/> implements both <see cref="IAsyncEnumerable{T}"/> and
/// <see cref="IAsyncDisposable"/>, which makes a plain <c>.ConfigureAwait(bool)</c> call ambiguous
/// (CS0121) because <c>TaskAsyncEnumerableExtensions</c> provides an overload for each interface.
/// This extension method resolves the ambiguity by explicitly routing to the
/// <see cref="IAsyncEnumerable{T}"/> overload, which is the behavior callers need when using
/// <c>await foreach</c>.
/// </para>
/// </remarks>
/// <typeparam name="TEventArgs">The event-args type produced by the stream.</typeparam>
/// <param name="stream">The event stream to configure.</param>
/// <param name="continueOnCapturedContext">
/// <see langword="true"/> to capture and marshal continuation back to the original context;
/// <see langword="false"/> to continue on a thread-pool thread.
/// </param>
/// <returns>A configured enumerable that applies the specified context-capture behavior.</returns>
public static ConfiguredCancelableAsyncEnumerable<TEventArgs> ConfigureAwait<TEventArgs>(
this IEventStream<TEventArgs> stream,
bool continueOnCapturedContext)
where TEventArgs : EventArgs
=> ((IAsyncEnumerable<TEventArgs>)stream).ConfigureAwait(continueOnCapturedContext);
}
84 changes: 84 additions & 0 deletions dotnet/test/webdriver/BiDi/EventStreamExtensionsTests.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
// <copyright file="EventStreamExtensionsTests.cs" company="Selenium Committers">
// Licensed to the Software Freedom Conservancy (SFC) under one
// or more contributor license agreements. See the NOTICE file
// distributed with this work for additional information
// regarding copyright ownership. The SFC licenses this file
// to you under the Apache License, Version 2.0 (the
// "License"); you may not use this file except in compliance
// with the License. You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing,
// software distributed under the License is distributed on an
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
// KIND, either express or implied. See the License for the
// specific language governing permissions and limitations
// under the License.
// </copyright>

using System.Runtime.CompilerServices;
using OpenQA.Selenium.BiDi;

namespace OpenQA.Selenium.Tests.BiDi;

[Parallelizable(ParallelScope.All)]
[FixtureLifeCycle(LifeCycle.InstancePerTestCase)]
class EventStreamExtensionsTests
{
private IBiDi _bidi;
private FakeTransport _transport;

[SetUp]
public async Task SetUp()
{
_transport = new FakeTransport();
_bidi = await Selenium.BiDi.BiDi.ConnectAsync(new Uri("ws://fake"), opts => opts.UseTransport(() => _transport));
}

[TearDown]
public async Task TearDown()
{
await _bidi.DisposeAsync();
}

[Test]
public async Task ConfigureAwait_ReturnsConfiguredCancelableAsyncEnumerable()
{
var stream = await _bidi.Script.RealmDestroyed.StreamAsync()
.WithResponse(_transport, """{"subscription":"sub-1"}""");

// This line previously failed to compile (CS0121) because IEventStream<T> implements
// both IAsyncEnumerable<T> and IAsyncDisposable and both have a matching ConfigureAwait overload.
// EventStreamExtensions.ConfigureAwait disambiguates toward IAsyncEnumerable<T>.
var configured = stream.ConfigureAwait(false);

Assert.That(configured, Is.InstanceOf<ConfiguredCancelableAsyncEnumerable<BiDi.Script.RealmDestroyedEventArgs>>());

await stream.DisposeAsync().WithResponse(_transport);
}

[Test]
public async Task ConfigureAwait_DeliverEventsThroughConfiguredEnumerable()
{
var stream = await _bidi.Script.RealmDestroyed.StreamAsync()
.WithResponse(_transport, """{"subscription":"sub-1"}""");

_transport.EnqueueEvent("script.realmDestroyed", """{"realm":"r-1"}""");

var received = new List<BiDi.Script.RealmDestroyedEventArgs>();

using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(5));

await foreach (var e in stream.ConfigureAwait(false).WithCancellation(cts.Token))
{
received.Add(e);
break;
}

Assert.That(received, Has.Count.EqualTo(1));
Assert.That(received[0].Realm.Id, Is.EqualTo("r-1"));

await stream.DisposeAsync().WithResponse(_transport);
}
}
9 changes: 9 additions & 0 deletions rb/lib/selenium/webdriver/common/websocket_connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,13 @@ class WebSocketConnection
RESPONSE_WAIT_INTERVAL = 0.1

MAX_LOG_MESSAGE_SIZE = 9999
MAX_FRAME_SIZE = 100 * 1024 * 1024 # 100 MB; DevTools payloads can be large

def initialize(url:)
# websocket-ruby exposes max_frame_size only as a global; bump it for devtools use
# only when the current limit is lower so user-configured values are not overridden
WebSocket.max_frame_size = MAX_FRAME_SIZE if WebSocket.max_frame_size < MAX_FRAME_SIZE

@callback_threads = ThreadGroup.new

@callbacks_mtx = Mutex.new
Expand Down Expand Up @@ -142,6 +147,10 @@ def attach_socket_listener
@callback_threads.add(callback_thread(message['params'], &callback))
end
end

# websocket-ruby rescues TooLong internally and returns nil from next;
# raise the stored error so the loop exits instead of spinning at 100% cpu
raise incoming_frame.error if incoming_frame.error?
Comment on lines +151 to +153

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

3. Listener failure causes timeouts 🐞 Bug ☼ Reliability

When attach_socket_listener raises incoming_frame.error, the surrounding rescue only logs and
exits the listener thread without closing the socket or surfacing the error to waiting callers.
After the listener exits, send_cmd can still successfully write but then waits until its 30s
timeout because responses are never enqueued.
Agent Prompt
## Issue description
The listener thread can terminate on `incoming_frame.error`, but the error is only logged; callers waiting in `send_cmd` are not notified and may block until `Wait` times out.

## Issue Context
- `send_cmd` writes to the socket and then waits for `messages.delete(id)`.
- `attach_socket_listener` rescues and logs but does not close the socket or store/propagate the failure.

## Fix Focus Areas
- rb/lib/selenium/webdriver/common/websocket_connection.rb[104-118]
- rb/lib/selenium/webdriver/common/websocket_connection.rb[131-158]

### Suggested fix
1. In `attach_socket_listener`, when an error occurs (either `incoming_frame.error?` path or in the rescue), store it on the connection (e.g., `@listener_error = e`) and transition the connection to a closed/closing state.
2. Close the underlying socket in that error path so subsequent `send_cmd` writes fail fast.
3. In `send_cmd`’s wait loop, check `@listener_error` (or `@closing`) and raise immediately instead of waiting for the timeout.

Example sketch:
- In listener rescue: set `@listener_error ||= e`; set `@closing = true`; `socket.close rescue nil`.
- In `send_cmd` wait block: `raise @listener_error if @listener_error` before/alongside checking `messages.delete(id)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

end
rescue *CONNECTION_ERRORS, WebSocket::Error => e
WebDriver.logger.debug "WebSocket listener closed: #{e.class}: #{e.message}", id: :ws
Expand Down
2 changes: 2 additions & 0 deletions rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ module Selenium

MAX_LOG_MESSAGE_SIZE: Integer

MAX_FRAME_SIZE: Integer

def initialize: (url: String) -> void

def add_callback: (untyped event) { () -> void } -> Integer
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# frozen_string_literal: true

# Licensed to the Software Freedom Conservancy (SFC) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The SFC licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing,
# software distributed under the License is distributed on an
# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.

require File.expand_path('../spec_helper', __dir__)

module Selenium
module WebDriver
describe WebSocketConnection do
# Stub network I/O so we can test initialize without a real socket.
before do
allow_any_instance_of(described_class).to receive(:process_handshake)
allow_any_instance_of(described_class).to receive(:attach_socket_listener).and_return(nil)
end

describe 'MAX_FRAME_SIZE' do
it 'is 100 MB' do
expect(described_class::MAX_FRAME_SIZE).to eq(100 * 1024 * 1024)
end
end

describe '#initialize' do
it 'raises WebSocket.max_frame_size to MAX_FRAME_SIZE when current value is lower' do
original = WebSocket.max_frame_size
WebSocket.max_frame_size = 1024

described_class.new(url: 'ws://localhost:4444')

expect(WebSocket.max_frame_size).to eq(described_class::MAX_FRAME_SIZE)
ensure
WebSocket.max_frame_size = original
end

it 'does not lower WebSocket.max_frame_size when already above MAX_FRAME_SIZE' do
original = WebSocket.max_frame_size
higher = described_class::MAX_FRAME_SIZE * 2
WebSocket.max_frame_size = higher

described_class.new(url: 'ws://localhost:4444')

expect(WebSocket.max_frame_size).to eq(higher)
ensure
WebSocket.max_frame_size = original
end
end
end
end
end