4.x: Use Schedule calls with state #558
Conversation
| @@ -56,9 +56,11 @@ protected override IDisposable SubscribeCore(IObserver<TSource> observer) | |||
| var d = new SerialDisposable(); | |||
| d.Disposable = m; | |||
|
|
|||
| m.Disposable = scheduler.Schedule(() => | |||
| m.Disposable = scheduler.Schedule((scheduler, source, observer, d), | |||
There was a problem hiding this comment.
If you are using this overload, the scheduler will be passed. Convention throughout the rest of the code is to name it 'self' (now '_'). You can then omit it from the state-tuple.
There was a problem hiding this comment.
This was an odd case, wrapping the outer scheduler. I'm not sure if the inner scheduler would be the correct one in this situation.
There was a problem hiding this comment.
It is generally just the executing one.
| @@ -414,7 +414,7 @@ private void DrainQueue(ICancelable cancel) | |||
| if (shouldWait) | |||
| { | |||
| var timer = new ManualResetEventSlim(); | |||
| _scheduler.Schedule(waitTime, () => { timer.Set(); }); | |||
| _scheduler.Schedule(timer, waitTime, (_, state) => { state.Set(); return Disposable.Empty; }); | |||
There was a problem hiding this comment.
Consider using the extension I recently added. You may omit the scheduler parameter (_), won't have to return Disposable.Empty everywhere.
Also, please give the inner variable a revealing name. I used to go for 'closureXYZ', e.g. closureTimer in this case. Just 'state' is not revealing.
There was a problem hiding this comment.
The problem with that overload is that the parameters are after the lambda, which I find harder to read.
Naming it like closureTimer makes little sense as these are not captured values by the lambda like before, they are now arguments.
There was a problem hiding this comment.
Changing the signature creates ambiguities unfortunately.
If not 'closureTimer', at least something more revealing than 'state'.
There was a problem hiding this comment.
I'll rename these. You could introduce an internal extension under a different name but with the same ordering of arguments.
There was a problem hiding this comment.
I can rename it in a subsequent PR and let ReSharper do the reordering. Would you then use the Action-extension for now?
There was a problem hiding this comment.
I find the ordering of the parameters distracting with those Action overloads. I'd prefer this ordering and a renamed internal schedule method.
There was a problem hiding this comment.
Ok I will file a PR with a renamed method and then we can get the Disposable.Empty out of that?
| @@ -284,12 +284,19 @@ public IDisposable Connect(IObserver<TArgs> observer) | |||
| { | |||
| if (--_count == 0) | |||
| { | |||
| _parent._scheduler.Schedule(_removeHandler.Dispose); | |||
| _parent._scheduler.Schedule(_removeHandler, (_, self) => RemoveHandlerDispose(self)); | |||
There was a problem hiding this comment.
'self', throughout calls to Schedule, has bee 'traditionally' used to denote the executing scheduler. If 'this' is passed as state, I like using @this as a name.
There was a problem hiding this comment.
I'll inline the method call back here.
| } | ||
|
|
||
| private void Invoke() | ||
| private IDisposable Invoke() |
There was a problem hiding this comment.
As above, consider the new extension and don't repeat returning Disposable.Empty.
This PR changes most of the no state argument
Schedulecalls into the state-awareSchedulecalls. This should help avoid instantiating a new delegate because of the variable capture.