-
Couldn't load subscription status.
- Fork 777
Add support for System.Threading.Lock #2254
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <Project> | ||
| <Import Project="..\Directory.Build.targets" /> | ||
| </Project> |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <Project> | ||
| <Import Project="..\Directory.Build.props" /> | ||
| </Project> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| <Project> | ||
| <Import Project="..\Directory.Build.targets" /> | ||
| </Project> |
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT License. | ||
| // See the LICENSE file in the project root for more information. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System.ComponentModel; | ||
| using System.Reactive.Disposables; | ||
|
|
@@ -229,7 +229,7 @@ public static IObservable<TSource> Synchronize<TSource>(IObservable<TSource> sou | |
| throw new ArgumentNullException(nameof(source)); | ||
| } | ||
|
|
||
| return new Synchronize<TSource>(source); | ||
| return new Synchronize<TSource, object>(source); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -252,9 +252,34 @@ public static IObservable<TSource> Synchronize<TSource>(IObservable<TSource> sou | |
| throw new ArgumentNullException(nameof(gate)); | ||
| } | ||
|
|
||
| return new Synchronize<TSource>(source, gate); | ||
| return new Synchronize<TSource, object>(source, gate); | ||
| } | ||
|
|
||
| #if HAS_LOCK_CLASS | ||
| /// <summary> | ||
| /// Wraps the source sequence in order to ensure observer callbacks are synchronized using the specified gate object. | ||
| /// </summary> | ||
| /// <typeparam name="TSource">The type of the elements in the source sequence.</typeparam> | ||
| /// <param name="source">Source sequence.</param> | ||
| /// <param name="gate">Gate object to synchronize each observer call on.</param> | ||
| /// <returns>The source sequence whose outgoing calls to observers are synchronized on the given gate object.</returns> | ||
| /// <exception cref="ArgumentNullException"><paramref name="source"/> or <paramref name="gate"/> is <c>null</c>.</exception> | ||
| public static IObservable<TSource> Synchronize<TSource>(IObservable<TSource> source, Lock gate) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the only additional public method required. Unless I've missed it, you've not added an equivalent extra overload in the Oh you'll also need to rerun the homoiconicity project to generate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot about the whole I have basically no knowledge of anything related to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The basic idea behind IObservable<int> src = GetSomeObservable();
IObservable<int> xs = src
.Where(x => x > 0)
.Select(x => x * 2);then IQbservable<int> xs = src
.AsQbservable()
.Where(x => x > 0)
.Select(x => x * 2);
Console.WriteLine(xs.Expression);then we've basically got the same query but this time as an (The So you can see that this thing knows that this is a So the basic idea here is that an
The idea behind Admittedly, I'm not aware of any public libraries that actually do that. But it's a model we support, and it's also the basis of distributed query execution in Reaqtor (https://reaqtive.net/). So there are automated tests that check that the public API we define for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I suspected it might be something like that. And you're saying there's no actual implementations here, just the modeling. I can work with that. |
||
| { | ||
| if (source == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(source)); | ||
| } | ||
|
|
||
| if (gate == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(gate)); | ||
| } | ||
|
|
||
| return new Synchronize<TSource, Lock>(source, gate); | ||
| } | ||
| #endif | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT License. | ||
| // See the LICENSE file in the project root for more information. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Reactive.Concurrency; | ||
|
|
@@ -37,7 +37,10 @@ public void Synchronization_Synchronize_ArgumentChecking() | |
| { | ||
| ReactiveAssert.Throws<ArgumentNullException>(() => Synchronization.Synchronize(default(IObservable<int>))); | ||
| ReactiveAssert.Throws<ArgumentNullException>(() => Synchronization.Synchronize(default(IObservable<int>), new object())); | ||
| ReactiveAssert.Throws<ArgumentNullException>(() => Synchronization.Synchronize(DummyObservable<int>.Instance, null)); | ||
| ReactiveAssert.Throws<ArgumentNullException>(() => Synchronization.Synchronize(DummyObservable<int>.Instance, null as object)); | ||
| #if HAS_LOCK_CLASS | ||
| ReactiveAssert.Throws<ArgumentNullException>(() => Synchronization.Synchronize(DummyObservable<int>.Instance, null as Lock)); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although this is fine as far as it goes, you've not added any behavioural tests. You need to verify that it really does acquire the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've got no issue with writing new functional tests, I just figured I'd start by following the pattern of what the existing operator has, which is nothing. Like, perhaps testing for thread lock mechanics was deemed not worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's not quite as bad as that, it's just that the tests are not where you thought they were (probably because you hadn't realised that Rx strangely offers two completely different APIs for using this: the The main tests for this code all go in through the LINQ operator, and can be found in That said, now that I look at these tests they aren't exactly great. This is the first time I've looked at this part of the code since taking over. If I were implementing this new feature, I'd want to expand the tests that verify the behaviour for the existing code too. I'd consider that to be an important part of ensuring that the behaviour is consistent across these two variations. The most important things to test would be:
There's a third scenario in which a no-arguments However...I've just discovered we actually rely on this rogue source protection internally! Looking in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I wouldn't say I missed its existence, but I definitely forgot to walk through the
I like you. I know too many people who would call this "scope creep".
Fully agreed, with direct access to the
Yup, I've encountered this in DynamicData, and we actually have testing fixtures to detect rogue sources.
The existing implementation sure doesn't look like it would do this. It just hands off to
I think |
||
| #endif | ||
| } | ||
|
|
||
| private class MySyncCtx : SynchronizationContext | ||
|
|
||
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 don't think this will do what you intend. I think you are assuming that then this type gets instantiated with
TGateset toLock, thelockkeyword usage inside the sink'sOnNext,OnError, andOnCompletedwill use the new behaviour for theLocktype, and when it is instantiated withobject, it will use the oldMonitor-based behaviour.As far as I can tell this is not what happens. If you write a generic type like this and then use the
lockkeyword on some expression of typeTGate, the compiler will emit the old-stylelockcode. E.g., in ILDASM I see this:So no matter what type
TGategets instantiated as, we're going to get the old-styleMonitor-based behaviour forlock.As far as I know, the C# compiler will only generate the new
System.Threading.Lockbased code for alockin cases where it knows at compile time that the expression is definitely of typeLock, which won't necessarily be the case here.(Remember that the compiler is going to emit exactly one implementation of
OnNext,OnError, andOnCompletedin the sink class. The instantiation for specific type arguments happens at runtime, but the IL generation has already happened by then. And the behaviour oflock—do you get theMonitoror theLockversion—is determined at compile time. It generates different IL for these two cases.)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.
Yup, really good catch. I was simply hoping to avoid duping the whole
Producerimplementation. I'll rewrite it.