Skip to content
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

fix: restrict access to _activeInvocations in concurrent RouteHandler methods … #2887

Merged
merged 4 commits into from
Mar 26, 2024

Conversation

Brimerland
Copy link
Contributor

… because HashSet is not thread-safe

Access to RouteHandle._activeInvocations must be controlled using a lock. Otherwise IndexOutOfBounds exceptions or InvalidOperation exceptions may be triggered during concurrent requests. HashSet is not thread safe.

A test "ShouldNotThrowOnConcurrentRequest" is added which reproduces the issue by having 10 pages requesting a http resource at high load for 5 seconds.

… methods, because HashSet is not thread-safe

Access to RouteHandle._activeInvocations must be controlled using a lock. Otherwise IndexOutOfBounds exceptions or InvalidOperation exceptions may be triggered during concurrent requests. HashSet is not thread safe.
@Brimerland
Copy link
Contributor Author

@microsoft-github-policy-service agree

@mxschmitt
Copy link
Member

What do you think about the following approach instead? Thats how we usually deal with such issues:

diff --git a/src/Playwright/Core/RouteHandler.cs b/src/Playwright/Core/RouteHandler.cs
index 10d27376..42214217 100644
--- a/src/Playwright/Core/RouteHandler.cs
+++ b/src/Playwright/Core/RouteHandler.cs
@@ -23,6 +23,7 @@
  */
 
 using System;
+using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Text.RegularExpressions;
 using System.Threading.Tasks;
@@ -32,7 +33,7 @@ namespace Microsoft.Playwright.Core;
 
 internal class RouteHandler
 {
-    private readonly HashSet<HandlerInvocation> _activeInvocations = new HashSet<HandlerInvocation>();
+    private readonly IDictionary<HandlerInvocation, byte> _activeInvocations = new ConcurrentDictionary<HandlerInvocation, byte>();
 
     private bool _ignoreException;
 
@@ -88,7 +89,7 @@ internal class RouteHandler
             Complete = new TaskCompletionSource<bool>(),
             Route = route,
         };
-        _activeInvocations.Add(handlerInvocation);
+        _activeInvocations.Add(handlerInvocation, 0);
         try
         {
             return await HandleInternalAsync(route).ConfigureAwait(false);
@@ -122,7 +123,7 @@ internal class RouteHandler
         else
         {
             var tasks = new List<Task>();
-            foreach (var activation in _activeInvocations)
+            foreach (var activation in _activeInvocations.Keys)
             {
                 if (!activation.Route.DidThrow)
                 {

@mxschmitt mxschmitt merged commit 55d726a into microsoft:main Mar 26, 2024
12 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants