Skip to content

Conversation

@AArnott
Copy link
Member

@AArnott AArnott commented Aug 12, 2018

In this PR, I promote our existing SingleThreadedSynchronizationContext test class to a product class. Along the way, I added a bunch of tests focused on the class and fixed a few issues they found.

Since we still want to use WPF's SynchronizationContext when we can as part of testing, I kept the original class in the test project, but renamed it to avoid a name collision. That caused a lot of noisy diffs in the test project. The only interesting changes in the test project to review are to the sync context class itself and the tests for it.

Closes #323

@AArnott AArnott added this to the v16.0 milestone Aug 12, 2018
@AArnott AArnott self-assigned this Aug 12, 2018
@AArnott AArnott requested a review from sharwell August 12, 2018 16:01
@AArnott
Copy link
Member Author

AArnott commented Aug 12, 2018

CC: @weltkante

}

/// <inheritdoc/>
public override SynchronizationContext CreateCopy() => this;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand it correctly CreateCopy isn't supposed to reuse the instance because reference equality of SynchronizationContext is used to inline invocations in some code paths. This could cause inlining invocations on the wrong thread if SynchronizationContext is flown to a different thread via ExecutionContext. At least thats what I read out of the comments in the WPF SynchronizationContext (comment on the compat switch here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment link. I didn't expect other code to use reference equality to mean they could skip using the SyncContext. Yikes. I'll fix this.

@weltkante
Copy link
Contributor

Generally looks good for my usage, but I'm not sure about semantics of CreateCopy. When researching the WPF implementation I noticed they had originally returned this as well and later had to fix it. See comment above with link to reference source.

@AArnott AArnott merged commit 0fb2e19 into microsoft:master Aug 20, 2018
@AArnott AArnott deleted the fix323 branch August 20, 2018 14:25
AArnott pushed a commit that referenced this pull request Feb 10, 2025
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Offer a single-threaded SynchronizationContext implementation

2 participants