-
Notifications
You must be signed in to change notification settings - Fork 152
Document how a library can depend on JTF #322
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
Conversation
|
|
||
| This pattern and self-initializer allows all the rest of your library code to assume JTF is always present (so you can use JTF.Run and JTF.RunAsync everywhere w/o feature that JTF will be null), and it mitigates all the deadlocks possible given the host constraints. | ||
|
|
||
| Note that when you create your own default instance of JoinableTaskContext (i.e. when the host doesn't), it will consider the thread you're on to be the main thread. If SynchronizationContext.Current != null it will capture it and use it to switch to the main thread when you ask it to (very similar to how VS works today), otherwise any request to SwitchToMainThreadAsync will never switch the thread (since no `SynchronizationContext` was supplied to do so) but otherwise JTF continues to work. |
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.
… otherwise any request to SwitchToMainThreadAsync will never switch the thread ...
❓ For a situation like this, is it possible to configure vs-threading so requests to switch to the main thread throw either InvalidOperationException or NotSupportedException? I'm specifically thinking of something like this:
gitextensions/gitextensions@d16afbd
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.
When would we configure it to do this?
As a default, having it no-op is great because it means unit tests "just work".
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.
As a default, having it no-op is great because it means unit tests "just work".
They can't "just work" if they are dependent on something which is not available during testing. Immediate failure to switch (exception) results in tests that more accurately reflect reality.
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.
They can't "just work" if they are dependent on something which is not available during testing
From my interactions with many partner teams who have or are migrating to JTF, they either mock their dependencies or are using MPF which isn't thread-safe but is free-threaded. In both of these cases, allowing await JTF.SwitchToMainThreadAsync() to no-op during testing is safe, easier than mocking up a UI thread, and readily acceptable to them when they ask about the behavior. If they really do want to test UI thread switching (e.g. check for deadlocks) this can be done, and we should prepare a best practices doc for this.
At this point changing JTF behavior when no SyncContext is present would be a breaking change anyway, so debating it is somewhat academic. I'm calling it out in the doc though specifically so folks can be aware of it. If they never expect a null SyncContext they can certainly raise an exception at this point. It's interesting timing that you object to this behavior since we just found a bug in VS where command line builds (devenv /build some.sln) will occasionally fail because we don't set a SyncContext on the main thread for them, leading SwitchToMainThreadAsync to quietly no-op. 😲
All we have so far is this which isn't very complete. Perhaps when we build up the testing docs we can discuss the default behavior and how to do whatever we decide is most appropriate for testing scenarios.
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 has bitten me too when using JTF in a .NET Core console application. It would be very useful to have a single threaded SynchronizationContext implementation "out of the box" instead of having to roll your own. I've started implementing one for my .NET Core project because I really need a "SwitchToMainThreadAsync" for certain patterns, like startup/shutdown, and .NET Core/JTF doesn't provide a single threaded SC out of the box.
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.
a DenyExecutionSynchronizationContext (some variant of the one shown in my commit linked above) to reject work requests for the UI thread.
@sharwell Why do you want your tests to fail when they execute product code that requires the UI thread?
@weltkante We can certainly define a single threaded sync context in the library. But I'm curious: what are you doing in .NET Core today that requires one? FYI in .NET Core 3.0 you'll have a couple to choose from since WinForms and WPF will be available.
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.
Why do you want your tests to fail when they execute product code that requires the UI thread?
I only want them to fail in this case when there is no UI thread. I would want them to fail because a precondition for testing the scenario (existence of the UI thread) has not been met. This is a more robust testing strategy which is also actionable.
- It is not difficult to provide a UI thread for use in these tests, in which case code would be able to run on the UI thread
- I do not make exceptions for false dependencies (e.g. code is using the UI thread for sequencing events that cannot execute concurrently) because this is an inappropriate synchronization strategy in interactive applications
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.
@AArnott I'm playing around with Span support on .NET Core and implementing POP3 (as it is a pretty simple protocol). Since it is not constrained to Windows I assume I'll probably be unable to use WinForms/WPF synchronization contexts.
As far as usage patterns go, I prefer giving objects that aren't threadsafe a "thread affinity" by switching to the main thread instead of locking around them. Using JTF and thread affinity turned out to be much less error prone than implementing locking everywhere. One recurring example being managing of the "application" object itself during startup and shutdown.
Anyway, if you are using JTF on .NET Core at all you'll run into the problem that you'll have no out-of-the box SynchronizationContext and can't use SwitchToMainThread (silently does nothing). Took quite a while to figure out what went wrong when I started using .NET Core.
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'm playing around with Span support on .NET Core and implementing POP3
Cool. @weltkante, are you implementing it with the new System.IO.Pipelines library? That leverages Span<T>/Memory<T> and is supposed to make writing such networking applications more efficient.
Thanks for the use case info. Thread affinity in lieu of locks isn't an approach I particularly endorse, but if it's acceptable for your application and it leads to fewer bugs, I'm not going to argue with you. Given the thread itself isn't special, but you simply want to avoid concurrent execution, have you considered using our AsyncSemaphore class or the brand new ReentrantSemaphore class as a way to eliminate concurrency instead of using thread affinity?
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.
@AArnott Yes, I'm using the POP3 project trying to get used to the Pipelines API and learning what patterns work well before doing a "real" project.
I'm aware of the async compatible synchronization objects in the library, but to be honest I don't have any idea how I would be debugging them when they deadlock. For prototyping I like the simplicity of thread affinity, checking why the main thread isn't responding always was easy to figure out. Reentrancy mostly hasn't been a problem since async/await makes it explicit and there are no COM apartments in .NET Core which could mess things up. If a thread affine object becomes a bottleneck, since the code is already async/await, I'd expect it not to be too hard to switch it over to explicit synchronization objects like async semaphore, maybe combined with object pooling if necessary.
Update .NET SDK and xunit.runner.visualstudio
No description provided.