Hi,
So the Roslyn compiler is kind of working in blazor wasm with the latest version (3.8.0-4.final) which is great! Appears the hashing issues are fixed. However, now I find there is a strange bug whereby I receive a multi threading exception under the following scenario:
Strangely it works if I close the original tab and open a new one.
Thanks
Last time Roslyn was working in the browser was with Blazor Webassembly 3.2.1 and Microsoft.CodeAnalysis 3.6.0
System.PlatformNotSupportedException: Cannot wait on monitors on this runtime.
at System.Threading.Monitor.ObjWait(Boolean exitContext, Int32 millisecondsTimeout, Object obj)
at System.Threading.Monitor.Wait(Object obj, Int32 millisecondsTimeout, Boolean exitContext)
at System.Threading.Monitor.Wait(Object obj, Int32 millisecondsTimeout)
at System.Threading.ManualResetEventSlim.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.SpinThenBlockingWait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.InternalWaitCore(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.InternalWait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
at System.Threading.Tasks.Task.Wait()
at System.Threading.Tasks.TaskReplicator.Replica.Wait()
at System.Threading.Tasks.TaskReplicator.Run[RangeWorker](ReplicatableUserAction`1 action, ParallelOptions options, Boolean stopOnFirstFailure)
at System.Threading.Tasks.Parallel.ForWorker[Object](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
at System.Threading.Tasks.Parallel.For(Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body)
at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(CompilationStage stage, Boolean includeEarlierStages, DiagnosticBag diagnostics, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(CompilationStage stage, Boolean includeEarlierStages, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.CompileMethods(CommonPEModuleBuilder moduleBuilder, Boolean emittingPdb, Boolean emitMetadataOnly, Boolean emitTestCoverageData, DiagnosticBag diagnostics, Predicate`1 filterOpt, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream metadataPEStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, CompilationTestData testData, CancellationToken cancellationToken)
at Microsoft.CodeAnalysis.Compilation.Emit(Stream peStream, Stream pdbStream, Stream xmlDocumentationStream, Stream win32Resources, IEnumerable`1 manifestResources, EmitOptions options, IMethodSymbol debugEntryPoint, Stream sourceLinkStream, IEnumerable`1 embeddedTexts, Stream metadataPEStream, CancellationToken cancellationToken)
I think this is by design -- Roslyn uses mechanisms that rely on mutexes, and those aren't supported on Blazor.
@marek-safar given there's only one thread in WASM, as I understand it, is there any kind of cooperative multithreading support, such that a mutex could work?
@danmosemsft But why would it work on first page load? Also to note it works when you close the original tab and create a new one.
I am guessing that if no task is already in progress, it has a special path that does not bother to check the mutex.
Although System.Threading.Monitor.Wait is unsupported API on wasm I'm reluctant to simply mark the issue as by design. We have Parallel.For implementation which does not work on wasm with no warning to the developers which is not great.
There two different solutions I can think of
@jeffhandley @buyaa-n I'm concern that #43363 did catch this issue. Could you please investigate why?
Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.
Tagging subscribers to this area: @tarekgh
See info in area-owners.md if you want to be subscribed.
@marek-safar I think it would be beneficial to go with the first option (of course I would say that :) ). The roslyn api is so close to working perfectly under wasm and I think a significant number of users are using this api within the browser. Would marking the API as unsupported mean even the features that were working wouldn't work?
Perhaps adding an option as an input parameter to disable multithreading could also work?
marking the API as unsupported mean even the features that were working wouldn't work
Marking API as unsupported would push the responsibility not to call this API to developers which is not a great option in this case (everyone you'd need to think about the else case in if Environment.ProcessorCount != 1)
We could fix Parallel.For implementation not to use any threads
If Environment.ProcessorCount == 1, Parallel.For should not end up using other threads. You see otherwise?
Oops, nevermind, I was thinking of PLINQ.
It can make sense for Parallel.For to use multiple threads even on a single core. The difference with wasm/blazor is it's not just single core but also single thread.
I was thinking of PLINQ.
Heh, me too. Tweaked my comment to focus on a single thread environment
I agree we should fix Parallel.For to not attempt to use multiple threads.
I suspect this is related to https://github.com/dotnet/runtime/issues/2078 as well, and that a fix to that would make this error go away without further changes to Parallel.For to try to special-case a system that only ever has a single thread.
Do we have API to detect whether only 1 thread is possible? (Environment.SingleThreaded or so)
Nope
You can always try/catch PNSE.
Do we have API to detect whether only 1 thread is possible?
What about ThreadPool.GetMaxThreads?
What about ThreadPool.GetMaxThreads?
In a console app, as an example, if GetMaxThreads said there was one worker thread, that still means the app has at least two threads on which it can execute code: the main thread and that single thread pool thread. With Blazor/wasm, they're one in the same.
For https://github.com/dotnet/runtime/pull/38690 what we did was add an internal bool Thread.IsThreadStartSupported that is hardcoded to false on Browser and true everywhere else. It's internal to System.Private.Corelib.dll but I guess we can just duplicate that in System.Threading.Tasks.Parallel.
if GetMaxThreads said there was one worker thread, that still means the app has at least two threads on which it can execute code: the main thread and that single thread pool thread
The app can have 100 threads but if TP is limited to single thread System.Threading.Tasks.Parallel.For should probably be optimized for sequential execution
The app can have 100 threads but if TP is limited to single thread System.Threading.Tasks.Parallel.For should probably be optimized for sequential execution
Parallel.For uses the current thread. If you run it from the main thread and there's also a thread pool thread, it should use both.
Hey does anyone have a rough ETA / update for this? Will it be expected to be fixed for main release coming up?
It won't be fixed in 5.0 rtm
so the questions are
Would a change that made Parallel.For handle not having support for ThreadStart (or Mutex, etc) be useful (defined as it would enable significant plausible Blazor workloads -- ie they would not simply hang or hit the next problem and instead work in some reasonable way)
If so, what are other places that could use /would need a similar change, eg PLINQ
If it does potentially make sense, what would the cost/risk potentially be
@stephentoub we talked offline - could you summarize your thoughts about these? It seems to me if this change was not a change we could take, we could close this now.
Looking at the stack above, Parallel.For attempts to take a mutex before it even tries to start a thread, so part of the work would be to avoid that.
Looking at the stack above, Parallel.For attempts to take a mutex before it even tries to start a thread
Not exactly. Parallel.For spreads itself across all available thread pool threads, and it does this via "replicas". You have a work item capable of repeatedly taking the next piece of work to be processed and executing the body delegate on it, but before it starts doing so, it queues another copy of itself, a replica, such that if another thread pool thread frees up, that thread can start running that replica, such that there are now two work items concurrently processing. And of course, the first thing that replica does before it starts processing is queues a replica, just in case another thread is available. Etc. The stack shown is likely the main thread having finished all of the work it can do, now waiting for the replica it queued to complete. Which is why I cited https://github.com/dotnet/runtime/issues/2078, where the issue is that we currently lack the public API surface area for the main thread to cancel the queued work item.
Would a change that made Parallel.For handle not having support for ThreadStart (or Mutex, etc) be useful
If for browser we're ok with saying that Parallel.For simply runs sequentially, then we should just build a browser-specific Parallel that's super simple, same surface area but just implemented with for / foreach loops. With size as a primary concern, anyone using Parallel is likely shipping most of the 41K its assembly brings, whereas the same surface area that doesn't do anything with concurrency is likely a small fraction of that.
If so, what are other places that could use /would need a similar change, eg PLINQ
Parallel and PLINQ are the two main ones.
If it does potentially make sense, what would the cost/risk potentially be
Cost for Parallel is probably a few days. There's ~50 overloads of For/ForEach/Invoke to stub out on top of sequential loops, ensuring it's built/shipped appropriately, updating tests as needed, etc.
That said, if our goal isn't about size and "just make this work", we could try building in a workaround like the one I outlined at https://github.com/dotnet/runtime/issues/2078#issuecomment-577725818 (i.e. using that TaskScheduler as an implementation detail instead of the default), just for browser (maybe under an OperatingSystem.IsBrowser check). Or we could implement https://github.com/dotnet/runtime/issues/17148, and use it from Parallel.For, in which case I expect this issue would also go away.
I don't have any good data but Parallel.For is probably still not commonly used so the size is not that big concern for this class.
In that case, can someone try https://github.com/stephentoub/runtime/commit/2e266a968075fd34907ff9483abc0016d2267401 to see if it addresses the issue?
@lewing could you follow up on that
@andersson09 do you have a sample to reproduce this case?
@lewing Will create a sample ready for Monday.
Some good news. Just updated to .net 5 release version and roslyn to latest 3.8.0 package before creating a sample and I no longer see this error :) Not exactly sure what fixed it, were @stephentoub changes merged into .net 5 release?
Thank you all
were @stephentoub Stephen Toub FTE changes merged into .net 5 release?
Nope. Most likely either whatever path was using Parallel.For is no longer doing so, or whatever path was using Parallel.For is now doing so from the thread pool, or there's some kind of race condition that just happens to not currently be manifesting.
Ok, @lewing shall I still create a sample of when it didn't work (RC2) or are you ok to test this in isolation away from the roslyn context?
@andersson09 if you can easily create a sample that failed with rc2 and the older roslyn I would definitely still be interested in looking at it. This path has changed a little in roslyn but not in a way that would obviously resolve what appeared to be the issue here.
@lewing presumably this should also be trigger by some of the existing tests
It almost sounds like you should just support this instead:
Our issue is exactly the opposite of @andersson09's, and happens in a different place, but seems to have the same root cause. Our call to CSharpCompilation.CreateScriptCompilation succeeds but the first time we try to do CSharpCompilation.GetDiagnostics following the compilation call we get the Cannot wait on monitors... exception (stack trace below). The second time we execute this exact same code path, it succeeds without throwing an exception.
The solutions proposed above to "fix" Parallel.For seem like they would fix this issue as well, but it would be nice to have a work-around. I see the work-around proposed here, but that doesn't help those of us who aren't calling Parallel.For directly. Perhaps the ability to pass through the ParallelOptions for all the CSharpCompilation methods that internally use Parallel.For, would enable us to use the proposed work-around.
Cannot wait on monitors on this runtime.
blazor.webassembly.js:1 at System.Threading.Monitor.ObjWait(Boolean exitContext, Int32 millisecondsTimeout, Object obj)
blazor.webassembly.js:1 at System.Threading.Monitor.Wait(Object obj, Int32 millisecondsTimeout, Boolean exitContext)
blazor.webassembly.js:1 at System.Threading.Monitor.Wait(Object obj, Int32 millisecondsTimeout)
blazor.webassembly.js:1 at System.Threading.ManualResetEventSlim.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
blazor.webassembly.js:1 at System.Threading.Tasks.Task.SpinThenBlockingWait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
blazor.webassembly.js:1 at System.Threading.Tasks.Task.InternalWaitCore(Int32 millisecondsTimeout, CancellationToken cancellationToken)
blazor.webassembly.js:1 at System.Threading.Tasks.Task.InternalWait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
blazor.webassembly.js:1 at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
blazor.webassembly.js:1 at System.Threading.Tasks.Task.Wait()
blazor.webassembly.js:1 at System.Threading.Tasks.TaskReplicator.Replica.Wait()
blazor.webassembly.js:1 at System.Threading.Tasks.TaskReplicator.Run[RangeWorker](ReplicatableUserAction`1 action, ParallelOptions options, Boolean stopOnFirstFailure)
blazor.webassembly.js:1 at System.Threading.Tasks.Parallel.ForWorker[Object](Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body, Action`2 bodyWithState, Func`4 bodyWithLocal, Func`1 localInit, Action`1 localFinally)
blazor.webassembly.js:1 at System.Threading.Tasks.Parallel.For(Int32 fromInclusive, Int32 toExclusive, ParallelOptions parallelOptions, Action`1 body)
blazor.webassembly.js:1 at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(CompilationStage stage, Boolean includeEarlierStages, DiagnosticBag diagnostics, CancellationToken cancellationToken)
blazor.webassembly.js:1 at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(CompilationStage stage, Boolean includeEarlierStages, CancellationToken cancellationToken)
blazor.webassembly.js:1 at Microsoft.CodeAnalysis.CSharp.CSharpCompilation.GetDiagnostics(CancellationToken cancellationToken)
blazor.webassembly.js:1 at ElementsWasm.Pages.Model.TryCompile(String source, CSharpCompilation& compilation, Assembly& assembly, List`1& errorDiagnostics) in /Users/ikeough/Documents/ElementsWasm/Pages/Model.razor:line 143