Skip to content

[dotnet] Thread safe product info determination - #17684

Merged
nvborisenko merged 2 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-thread-safe-resources
Jun 16, 2026
Merged

[dotnet] Thread safe product info determination#17684
nvborisenko merged 2 commits into
SeleniumHQ:trunkfrom
nvborisenko:dotnet-thread-safe-resources

Conversation

@nvborisenko

Copy link
Copy Markdown
Member

💥 What does this PR do?

This pull request refactors the ResourceUtilities class to improve thread safety and code clarity by replacing manual lazy initialization with the Lazy<T> type for static properties. It also removes some unused using directives for a cleaner codebase.

Refactoring for thread safety and clarity:

  • Replaced manual lazy initialization of ProductVersion and PlatformFamily with Lazy<string>, ensuring thread-safe, lazy-loaded static properties. (dotnet/src/webdriver/Internal/ResourceUtilities.cs)

Code cleanup:

  • Removed unused System.Diagnostics and System.Globalization using directives for a cleaner file. (dotnet/src/webdriver/Internal/ResourceUtilities.cs)

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s):
    • What was generated:
    • I reviewed all AI output and can explain the change

🔄 Types of changes

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

@qodo-code-review

qodo-code-review Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. NET462 build break 🐞 Bug ≡ Correctness
Description
ResourceUtilities.GetPlatformString() still references the Process type in the #if NET462 branch,
but this PR removed the System.Diagnostics import, causing the net462 target to fail to compile.
Code

dotnet/src/webdriver/Internal/ResourceUtilities.cs[20]

-using System.Diagnostics;
Evidence
The project targets net462, and the updated ResourceUtilities.cs still uses Process under `#if
NET462, but there is no System.Diagnostics` import in the file nor in global usings, so the symbol
will not resolve when compiling the net462 target.

dotnet/src/webdriver/Selenium.WebDriver.csproj[1-6]
dotnet/src/webdriver/Internal/ResourceUtilities.cs[20-22]
dotnet/src/webdriver/Internal/ResourceUtilities.cs[51-67]
dotnet/src/webdriver/Properties/GlobalUsings.cs[20-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dotnet/src/webdriver/Internal/ResourceUtilities.cs` still uses `Process` inside the `#if NET462` code path, but the PR removed `using System.Diagnostics;`. Since the WebDriver project targets `net462`, this will cause a compile error when building that target.

### Issue Context
- `Selenium.WebDriver.csproj` multi-targets `net462;netstandard2.0;net8.0`.
- The `Process` type is not imported via `GlobalUsings.cs`.

### Fix Focus Areas
- dotnet/src/webdriver/Internal/ResourceUtilities.cs[20-23]
- dotnet/src/webdriver/Internal/ResourceUtilities.cs[51-67]
- dotnet/src/webdriver/Properties/GlobalUsings.cs[20-27]

### Suggested fix
Add `using System.Diagnostics;` back, ideally guarded to only apply when needed:
```csharp
#if NET462
using System.Diagnostics;
#endif
```
Alternatively, fully-qualify the type usages (`System.Diagnostics.Process`) inside the `#if NET462` block.

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


Grey Divider

Previous review results

Review updated until commit 94912cc

Results up to commit b7e2d36


🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)


Action required
1. NET462 build break 🐞 Bug ≡ Correctness
Description
ResourceUtilities.GetPlatformString() still references the Process type in the #if NET462 branch,
but this PR removed the System.Diagnostics import, causing the net462 target to fail to compile.
Code

dotnet/src/webdriver/Internal/ResourceUtilities.cs[20]

-using System.Diagnostics;
Evidence
The project targets net462, and the updated ResourceUtilities.cs still uses Process under `#if
NET462, but there is no System.Diagnostics` import in the file nor in global usings, so the symbol
will not resolve when compiling the net462 target.

dotnet/src/webdriver/Selenium.WebDriver.csproj[1-6]
dotnet/src/webdriver/Internal/ResourceUtilities.cs[20-22]
dotnet/src/webdriver/Internal/ResourceUtilities.cs[51-67]
dotnet/src/webdriver/Properties/GlobalUsings.cs[20-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dotnet/src/webdriver/Internal/ResourceUtilities.cs` still uses `Process` inside the `#if NET462` code path, but the PR removed `using System.Diagnostics;`. Since the WebDriver project targets `net462`, this will cause a compile error when building that target.

### Issue Context
- `Selenium.WebDriver.csproj` multi-targets `net462;netstandard2.0;net8.0`.
- The `Process` type is not imported via `GlobalUsings.cs`.

### Fix Focus Areas
- dotnet/src/webdriver/Internal/ResourceUtilities.cs[20-23]
- dotnet/src/webdriver/Internal/ResourceUtilities.cs[51-67]
- dotnet/src/webdriver/Properties/GlobalUsings.cs[20-27]

### Suggested fix
Add `using System.Diagnostics;` back, ideally guarded to only apply when needed:
```csharp
#if NET462
using System.Diagnostics;
#endif
```
Alternatively, fully-qualify the type usages (`System.Diagnostics.Process`) inside the `#if NET462` block.

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


Qodo Logo

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jun 16, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Make ResourceUtilities product/platform info lazy and thread-safe
🐞 Bug fix ✨ Enhancement 🕐 10-20 Minutes

Grey Divider

Description

• Replace manual static caching with Lazy for thread-safe ProductVersion/PlatformFamily
• Simplify ProductVersion resolution from AssemblyInformationalVersionAttribute with a safe fallback
• Remove unused imports and legacy resource-stream helper from this partial file
Diagram

graph TD
  A["Callers"] --> B["ResourceUtilities"] --> C["Lazy version"] --> D["Assembly metadata"]
  B --> E["Lazy platform"] --> F["GetPlatformString()"] --> G["RuntimeInformation"]
  F --> H["Process uname (NET462)"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. LazyInitializer.EnsureInitialized fields
  • ➕ Thread-safe lazy init without allocating Lazy instances
  • ➕ Keeps underlying storage as simple nullable fields
  • ➖ More verbose and error-prone (ref params/nullability) than Lazy
  • ➖ Harder to read/maintain for multiple properties
2. Eager static readonly initialization
  • ➕ Simplest code; no Lazy overhead
  • ➕ Initialization happens deterministically at type load
  • ➖ Computes values even if never used
  • ➖ Potentially undesirable if platform/version probing ever becomes more expensive

Recommendation: Lazy is a good fit here for clarity and thread-safety. However, validate NET462 compilation: this file still references System.Diagnostics.Process inside the NET462 block of GetPlatformString(), so removing using System.Diagnostics; likely breaks that target unless Process is imported elsewhere (e.g., global using) or fully qualified.

Files changed (1) +10 / -76

Bug fix (1) +10 / -76
ResourceUtilities.csRefactor product/platform info caching to Lazy<T> +10/-76

Refactor product/platform info caching to Lazy<T>

• Introduces Lazy<string> fields to compute ProductVersion and PlatformFamily exactly once in a thread-safe way. Simplifies ProductVersion derivation from assembly metadata and removes unused usings and an older resource-stream helper from this partial file.

dotnet/src/webdriver/Internal/ResourceUtilities.cs

Comment thread dotnet/src/webdriver/Internal/ResourceUtilities.cs
@qodo-code-review

qodo-code-review Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Code review by qodo was updated up to the latest commit 94912cc

@nvborisenko
nvborisenko merged commit 1ac1af6 into SeleniumHQ:trunk Jun 16, 2026
32 of 34 checks passed
@nvborisenko
nvborisenko deleted the dotnet-thread-safe-resources branch June 16, 2026 11:11
This was referenced Jul 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-dotnet .NET Bindings

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants