Home Original page

[dotnet] [bidi] Hide Broker as internal implementation by nvborisenko · Pull Request #16982 · SeleniumHQ/selenium

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Relax result type constraint

Relax the generic constraint on ExecuteCommandAsync from where TResult :
EmptyResult to where TResult : class to allow all valid result types and fix a
compilation error.

dotnet/src/webdriver/BiDi/Module.cs [33-35]

 protected Task<TResult> ExecuteCommandAsync<TCommand, TResult>(TCommand command, CommandOptions? options, JsonTypeInfo<TCommand> jsonCommandTypeInfo, JsonTypeInfo<TResult> jsonResultTypeInfo)
     where TCommand : Command
-    where TResult : EmptyResult
+    where TResult : class
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical compilation error. The generic constraint where TResult : EmptyResult is incorrect for most command results, and relaxing it is necessary for the code to compile and function as intended.

High
Fix array literal syntax

Replace the invalid array literal syntax [eventName] with a valid array creation
expression, such as new[] { eventName }, to fix a compilation error.

dotnet/src/webdriver/BiDi/Broker.cs [155]

-var subscribeResult = await _bidi.SessionModule.SubscribeAsync([eventName], new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }).ConfigureAwait(false);
+var subscribeResult = await _bidi.SessionModule.SubscribeAsync(new[] { eventName }, new() { Contexts = options?.Contexts, UserContexts = options?.UserContexts }).ConfigureAwait(false);

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies invalid C# syntax ([eventName]) that will cause a compilation failure. The proposed fix to use new[] { eventName } is the correct way to create an array inline and is essential for the code to compile.

High
Synchronize handler addition

Wrap the addition of an event handler to the handlers list within a lock
statement to prevent race conditions and ensure thread-safe modification.

dotnet/src/webdriver/BiDi/Broker.cs [153-157]

-var handlers = _eventHandlers.GetOrAdd(eventName, (a) => []);
+var handlers = _eventHandlers.GetOrAdd(eventName, _ => new List<EventHandler>());
 ...
-handlers.Add(eventHandler);
+lock (handlers)
+{
+    handlers.Add(eventHandler);
+}

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 8

__

Why: This suggestion correctly identifies a potential race condition when adding event handlers from multiple threads. Using a lock is a valid and important fix to ensure thread safety and prevent data corruption in a concurrent environment.

Medium
Learned
best practice
Validate inputs before use

Validate eventName for null/whitespace and validate the delegate/type info
arguments for null before constructing handlers and calling into Broker to
harden the API boundary.

dotnet/src/webdriver/BiDi/Module.cs [40-52]

 protected Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Action<TEventArgs> action, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
     where TEventArgs : EventArgs
 {
+    ArgumentException.ThrowIfNullOrWhiteSpace(eventName);
+    ArgumentNullException.ThrowIfNull(action);
+    ArgumentNullException.ThrowIfNull(jsonTypeInfo);
+
     var eventHandler = new SyncEventHandler<TEventArgs>(eventName, action);
     return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo);
 }
 
 public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
     where TEventArgs : EventArgs
 {
+    ArgumentException.ThrowIfNullOrWhiteSpace(eventName);
+    ArgumentNullException.ThrowIfNull(func);
+    ArgumentNullException.ThrowIfNull(jsonTypeInfo);
+
     var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func);
     return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Add validation and null/blank guards at integration boundaries before using inputs (e.g., event names, handlers) to prevent unexpected runtime errors.

Low
General
Make subscription helper method protected

Change the SubscribeAsync method that accepts a Func<TEventArgs, Task> from
public to protected to match its Action counterpart, ensuring consistent
encapsulation.

dotnet/src/webdriver/BiDi/Module.cs [47-52]

-public Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
+protected Task<Subscription> SubscribeAsync<TEventArgs>(string eventName, Func<TEventArgs, Task> func, SubscriptionOptions? options, JsonTypeInfo<TEventArgs> jsonTypeInfo)
     where TEventArgs : EventArgs
 {
     var eventHandler = new AsyncEventHandler<TEventArgs>(eventName, func);
     return Broker.SubscribeAsync(eventName, eventHandler, options, jsonTypeInfo);
 }
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies an access modifier inconsistency between two similar helper methods, and changing it to protected improves encapsulation and API consistency.

Low
  • More