Skip to content

[dotnet] Fix design of raising of events when driver service starts/started #16083

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

Closed

Conversation

nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jul 23, 2025

User description

💥 What does this PR do?

Raising the events is simple task which should not be supposed to be overridden. So if some event happens - just raise it.

🔧 Implementation Notes

It removes protected method. I cannot image who uses it. Probably Appium, but no, they don't use it.

💡 Additional Considerations

It is beneficial, because now selenium doesn't allocate stupid memory if there are no subscribers on this events.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Enhancement


Description

  • Simplified event raising by removing protected wrapper methods

  • Direct event invocation with null-conditional operator

  • Reduced memory allocation when no event subscribers exist

  • Removed unnecessary argument validation and method overhead


Diagram Walkthrough

flowchart LR
  A["Protected OnDriverProcessStarting method"] --> B["Direct DriverProcessStarting?.Invoke()"]
  C["Protected OnDriverProcessStarted method"] --> D["Direct DriverProcessStarted?.Invoke()"]
  E["Argument validation overhead"] --> F["Streamlined event invocation"]
Loading

File Walkthrough

Relevant files
Enhancement
DriverService.cs
Streamlined event raising mechanism                                           

dotnet/src/webdriver/DriverService.cs

  • Replaced protected OnDriverProcessStarting and OnDriverProcessStarted
    methods with direct event invocation
  • Used null-conditional operator for safer event raising
  • Removed argument validation and method overhead
  • Simplified event architecture in driver service lifecycle
+3/-32   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jul 23, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Breaking Change

Removing protected methods OnDriverProcessStarting and OnDriverProcessStarted is a breaking change for any derived classes that override these methods. This could break existing code that extends DriverService and customizes event handling behavior.

this.DriverProcessStarting?.Invoke(this, new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo));

this.driverServiceProcess.Start();
bool serviceAvailable = this.WaitForServiceInitialization();

this.DriverProcessStarted?.Invoke(this, new DriverProcessStartedEventArgs(this.driverServiceProcess));
Null Reference

Direct event invocation creates event args objects even when no subscribers exist, contradicting the PR's claim about memory optimization. The null-conditional operator only prevents the invoke call, not the object creation.

this.DriverProcessStarting?.Invoke(this, new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo));

this.driverServiceProcess.Start();
bool serviceAvailable = this.WaitForServiceInitialization();

this.DriverProcessStarted?.Invoke(this, new DriverProcessStartedEventArgs(this.driverServiceProcess));

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Conditional event raising based on success

The DriverProcessStarted event should only be raised if the service successfully
starts and becomes available. Currently, it's raised regardless of whether
WaitForServiceInitialization() succeeds, which could mislead event handlers
about the actual service state.

dotnet/src/webdriver/DriverService.cs [248-251]

 this.driverServiceProcess.Start();
 bool serviceAvailable = this.WaitForServiceInitialization();
 
-this.DriverProcessStarted?.Invoke(this, new DriverProcessStartedEventArgs(this.driverServiceProcess));
+if (serviceAvailable)
+{
+    this.DriverProcessStarted?.Invoke(this, new DriverProcessStartedEventArgs(this.driverServiceProcess));
+}
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a logical flaw where the DriverProcessStarted event is raised before confirming the service is actually available, which could mislead event consumers.

Medium
  • More

@nvborisenko nvborisenko requested a review from Copilot July 23, 2025 20:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the event raising mechanism in the DriverService class by removing protected wrapper methods and using direct event invocation. The change eliminates unnecessary method overhead and reduces memory allocation when no event subscribers exist.

  • Replaced protected OnDriverProcessStarting and OnDriverProcessStarted methods with direct event invocation using null-conditional operator
  • Removed argument validation overhead and method indirection
  • Streamlined the driver service lifecycle event architecture

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants