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

Some tests depend on a SynchronizationContext #1885

Open
idg10 opened this issue Mar 8, 2023 · 3 comments
Open

Some tests depend on a SynchronizationContext #1885

idg10 opened this issue Mar 8, 2023 · 3 comments

Comments

@idg10
Copy link
Collaborator

idg10 commented Mar 8, 2023

The TaskLikeSupportTest.Basics test, and the EventLoop_ScheduleActionDue and EventLoop_ScheduleActionNested tests in EventLoopSchedulerTest were hanging sporadically after moving from xUnit to MSTest. This is indicative of some underlying problem—the success or failure of a test shouldn't depend on which particular test framework you are using.

One difference between the test frameworks is that xUnit always supplies a non-null SynchronizationContext.Current to tests (apparently to support async void tests). MSTest does not. (It supplies a code analyzer that detects async void tests and tells you not to use them.) The presence of a synchronization context can cause Rx to execute different code paths,.

This appears to be why the TaskLikeSupportTest.Basics test was failing. We've modified the code to set up a SynchronizationContext explicitly, and this appears to have got rid of the hange, but this is only a workaround. We need to establish whether there's something about the test that truly requires a SynchronizationContext for the code to be valid (which is possible, but if so, it's not obvious why), or whether we really should expect it to work with or without a SynchronizationContext. If it's the latter, that indicates a subtle bug somewhere, in which case the long term solution should be to have two forms of the test, one with the SynchronizationContext and one without, and make whatever fix is required for the test to pass reliably on both.

As for the two problematic tests in EventLoopSchedulerTest, these continued to hang even after explicitly setting up a SynchronizationContext. So whatever it is about xUnit that was different that they depended on, it apparently wasn't just the presence/absence of a SynchronizationContext.

For now, those two tests have been annotated with [Ignore] so that we can move forward, but these problems need to be fixed properly.

idg10 added a commit that referenced this issue Mar 8, 2023
Apparently supplying a SynchronizationContext isn't enough to prevent the EventLoop_ScheduleActionNested and EventLoop_ScheduleActionDue tests from hanging on the build agent.

We'll need to fix this properly, so #1885 will track this
@idg10 idg10 added this to the Rx 6.0 milestone Mar 8, 2023
@quinmars
Copy link
Contributor

I only know two operators that directly depend on the synchronization context, namely FromEvent and FromEventPattern. There is a nice explanation why they did this here. It looks like the synchronization context is only considered when there is no scheduler provided. Hence it would be probably enough to provide a scheduler explicitly.

@idg10
Copy link
Collaborator Author

idg10 commented Mar 28, 2023

I don't think it's operators that are the problem.

Some of the await/async integration (e.g., in AsyncSubject<T>) interacts with SynchronizationContext. This is why the TaskLikeSupportTest.Basics test behaves differently in the absence of a sync context. That test ends up in AsyncSubject<T>.AwaitObserver.InvokeOnOriginalContext, which uses SynchronizationContext.Post if it captured a SynchronizationContext, but just invokes its callback synchronously if it did not. This is a quite significant change in behaviour, and seems to be behind that test's intermittent failures when running on MSTest. The only operators that test uses are Return and Delay, but it's not the operators themselves that are the issue.

The operators you mention aren't ones I've seen in any test failures. (There are tests for these that set up a SynchronizationContext explicitly. But that's fine, because those deliberately have SynchronizationContext-related behaviour, as you've said, and so it's appropriate that the tests set them up explicitly. This is not what concerns me. What I'm concerned about is tests that seem to have accidentally depended on the test framework setting up a SynchronizationContext.)

So the thing I'm trying to work out is what the expected behaviour of the code in TaskLikeSupportTest.Basics should be in cases where SynchronizationContext.Current returns null. Currently, it hangs intermittently. It's unclear to me whether that's a bug in the implementation, or whether there's some good reason that you should never attempt anything like what the test does in the absence of a SynchronizationContext.

(I'm also still trying to work out why those two EventLoopScheduler tests sometimes fail. I don't yet know whether that's related to SynchronizationContext, or some other difference between the test frameworks.)

@idg10 idg10 modified the milestones: Rx 6.0, vNext May 17, 2023
@idg10
Copy link
Collaborator Author

idg10 commented Nov 2, 2023

I've discovered that reason BasicsNoSynchronizationContext fails intermittently is that something somewhere is apparently managing to set SynchronizationContext.Current to contain a WindowsFormsSynchronizationContext.

I guess this happens because some test somewhere created a Control-derived class, making Windows Forms decide that the test thread is in fact a UI thread, and I guess it sets up SyncrhonizationContext.Current at that point?

The effect of this on BasicsNoSynchronizationContext is that because there is in fact a sync context, the AsyncSubject<T> created by the await machinery tries to handle completion with a Post through that context, but because nothing's running a message loop, it hangs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants