-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[browser][MT] Log ManagedThreadId and NativeThreadId for known test failures #98291
Conversation
Tagging subscribers to 'arch-wasm': @lewing |
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -376,6 +377,7 @@ public async Task ThreadingTimer(Executor executor) | |||
await tcs.Task; | |||
}, cts.Token); | |||
|
|||
Console.WriteLine("ThreadingTimer: ManagedThreadId: " + Environment.CurrentManagedThreadId + " NativeThreadId: " + WebWorkerTestHelper.NativeThreadId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WebWorkerTest.ThreadingTimer
didn't hit at all in the log. So it would be also good to print timestamp before (line 367) and after (380)
Maybe we just need to move hit = true;
before tcs.SetResult();
on line 374, because that's race conditon in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this part accordingly.
@@ -451,7 +453,7 @@ public async Task WaitAssertsOnJSInteropThreads(Executor executor, NamedCall met | |||
{ | |||
exception = ex; | |||
} | |||
|
|||
Console.WriteLine("WaitAssertsOnJSInteropThreads: ManagedThreadId: " + Environment.CurrentManagedThreadId + " NativeThreadId: " + WebWorkerTestHelper.NativeThreadId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this tell us which and why Value is null
? #97914
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, we know which exception, we don't know why.
The "why" is likely because the call is executed on different thread than we expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this could also be the "why"
Line 159 in 5eddce6
} catch (OperationCanceledException) { /* ignore */ } |
This code is swallowing OperationCanceledException
instead of passing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added logs to validate if that is the case.
@@ -82,6 +82,7 @@ public async Task HttpClient_CancelInDifferentThread(Executor executor1, Executo | |||
{ | |||
CancellationTokenSource cts = new CancellationTokenSource(); | |||
var promise = response.Content.ReadAsStringAsync(cts.Token); | |||
Console.WriteLine("HttpClient_CancelInDifferentThread: ManagedThreadId: " + Environment.CurrentManagedThreadId + " NativeThreadId: " + WebWorkerTestHelper.NativeThreadId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use ThrowsAnyAsync<OperationCanceledException>
on line 81 may hide the problem.
The question is why we have 2 different code paths.
Please log stack traces in both cases, so that we could compare them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept ThrowsAsync<TaskCanceledException>
, so issue will still happen but added logs to see stack traces for both TaskCanceledException
and OperationCanceledException
.
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
...ervices.JavaScript.UnitTests/System/Runtime/InteropServices/JavaScript/WebWorkerTest.Http.cs
Show resolved
Hide resolved
@@ -156,14 +156,20 @@ public class NamedCall | |||
using var cts = new CancellationTokenSource(8); | |||
try { | |||
mr.Wait(cts.Token); | |||
} catch (OperationCanceledException) { /* ignore */ } | |||
} catch (OperationCanceledException) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should not be swallowed I think. Because it's not really the expected outcome.
If it was expected outcome, the outer assert should process that case, rather than misleading null, which for this test means that something is broken for Main and WebWorker executors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @radekdoulik
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to not swallow OperationCanceledException
.
/azp run runtime-wasm |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -156,14 +156,24 @@ public class NamedCall | |||
using var cts = new CancellationTokenSource(8); | |||
try { | |||
mr.Wait(cts.Token); | |||
} catch (OperationCanceledException) { /* ignore */ } | |||
} catch (Exception ex) { | |||
if (ex is OperationCanceledException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Radek's solution here is bit better #98508 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please observe the outcome and let us know what we learned from it. Thank you!
Log ManagedThreadId and NativeThreadId for following test failures
#98101, #98216, #97914
Once test failures will be fixed, we will remove these logs.