Skip to content

[dotnet] Fix race condition for .net framework when internal tracing enabled#16091

Merged
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-48-log-race
Jul 25, 2025
Merged

[dotnet] Fix race condition for .net framework when internal tracing enabled#16091
nvborisenko merged 2 commits intoSeleniumHQ:trunkfrom
nvborisenko:dotnet-48-log-race

Conversation

@nvborisenko
Copy link
Member

@nvborisenko nvborisenko commented Jul 25, 2025 •
edited by qodo-code-review bot
Loading

User description

🔗 Related Issues

using var driver = new ChromeDriver();

for (int 0 .. 100)
  driver.FindElement(By.CssSelector("div"));

Sometimes the exception is raised:

System.ArgumentException: An item with the same key has already been added.
   at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at System.Net.Http.Headers.HttpHeaders.AddHeaderToStore(String name, HeaderStoreItemInfo info)
   at System.Net.Http.Headers.HttpHeaders.CreateAndAddHeaderToStore(String name)
   at System.Net.Http.Headers.HttpHeaders.GetOrCreateHeaderInfo(String name, Boolean parseRawValues)
   at System.Net.Http.Headers.HttpHeaders.SetParsedValue(String name, Object value)
   at System.Net.Http.Headers.HttpContentHeaders.get_ContentLength()
   at System.Net.Http.HttpClientHandler.PrepareAndStartContentUpload(RequestState state)

💥 What does this PR do?

This exception might be happened in .NET Framework only. On .NET Core it works smoothly.

🔧 Implementation Notes

This is because we send and read HTTP Request Content in parallel. The implementation of network module in .NET Framework doesn't like it.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix

Description

  • Fix race condition in .NET Framework HTTP logging

  • Await HTTP response before reading request content

  • Add conditional compilation for .NET 8+ cancellation token

  • Prevent concurrent access to HTTP headers dictionary

Diagram Walkthrough

flowchart LR
  A["HTTP Request"] --> B["Send Request"]
  B --> C["Await Response"]
  C --> D["Read Request Content"]
  D --> E["Log Request"]
  E --> F["Log Response"]
Loading

File Walkthrough
Relevant files
Bug fix
HttpCommandExecutor.cs
Fix HTTP logging race condition                                                   

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs

  • Changed parallel execution to sequential for HTTP request/response
    logging
  • Added conditional compilation for .NET 8+ cancellation token support
  • Fixed race condition by awaiting response before reading request
    content
  • Improved thread safety for .NET Framework compatibility
 +10/-3   

@selenium-ci selenium-ci added the C-dotnet .NET Bindings label Jul 25, 2025
@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jul 25, 2025

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

Request Consumption

Reading request content after sending the request may consume the stream, potentially causing issues if the HTTP client needs to retry or if the content stream is not rewindable. This could lead to unexpected behavior in certain scenarios.

 
#if NET8_0_OR_GREATER
                var requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
#else
                var requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false);
#endif
                requestLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, requestContent);
            }
Error Handling

The sequential execution change means that if the response fails, the request content logging will still occur. Consider whether request logging should be skipped when the response indicates failure to avoid unnecessary processing.

 
            var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);

            StringBuilder requestLogMessageBuilder = new();
            requestLogMessageBuilder.AppendFormat(">> {0} RequestUri: {1}, Content: {2}, Headers: {3}",
                request.Method,
                request.RequestUri?.ToString() ?? "null",
                request.Content?.ToString() ?? "null",
                request.Headers?.Count());

            if (request.Content != null)
            {
#if NET8_0_OR_GREATER
                var requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
#else
                var requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false);
#endif
                requestLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, requestContent);
            }

            _logger.Trace(requestLogMessageBuilder.ToString());

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Jul 25, 2025 •
edited
Loading

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Read request content before sending

Reading request content after sending the request may fail because the content
stream could be consumed or disposed. Request content should be read before
sending the HTTP request to ensure it's available for logging.

dotnet/src/webdriver/Remote/HttpCommandExecutor.cs [452-460]

+string requestContent = null;
 if (request.Content != null)
 {
 #if NET8_0_OR_GREATER
-    var requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
+    requestContent = await request.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
 #else
-    var requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false);
+    requestContent = await request.Content.ReadAsStringAsync().ConfigureAwait(false);
 #endif
+}
+
+var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
+
+StringBuilder requestLogMessageBuilder = new();
+requestLogMessageBuilder.AppendFormat(">> {0} RequestUri: {1}, Content: {2}, Headers: {3}",
+    request.Method,
+    request.RequestUri?.ToString() ?? "null",
+    request.Content?.ToString() ?? "null",
+    request.Headers?.Count());
+
+if (requestContent != null)
+{
     requestLogMessageBuilder.AppendFormat("{0}{1}", Environment.NewLine, requestContent);
 }
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical bug where request.Content is read after being sent, which would likely fail as the content stream would already be consumed.

High
  • Update

@nvborisenko nvborisenko merged commit 3613f08 into SeleniumHQ:trunk Jul 25, 2025
10 checks passed
@nvborisenko nvborisenko mentioned this pull request Jul 31, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

C-dotnet .NET Bindings Review effort 2/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nvborisenko @selenium-ci