Skip to content

Ensure IsExecuting is false when command completes#714

Merged
anaisbetts merged 1 commit into
masterfrom
haacked/await-executeasync-race-condition-fix
Sep 4, 2014
Merged

Ensure IsExecuting is false when command completes#714
anaisbetts merged 1 commit into
masterfrom
haacked/await-executeasync-race-condition-fix

Conversation

@haacked
Copy link
Copy Markdown
Contributor

@haacked haacked commented Sep 4, 2014

Because ReactiveCommand decrements the in flight count in a Finally block, when we await a command's ExecuteAsync method, it still reports an in-flight count when the await returns. We want to not return until the in-flight count is decremented. This fixes that by putting the decrement in a Do statement for both the onNext block and the onError block.

Because ReactiveCommand decrements the in flight count in a Finally
block, when we await a command's ExecuteAsync method, it still reports
an in-flight count when the await returns. We want to not return until
the in-flight count is decremented. This fixes that.
anaisbetts pushed a commit that referenced this pull request Sep 4, 2014
…ce-condition-fix

Ensure IsExecuting is false when command completes
@anaisbetts anaisbetts merged commit 332295e into master Sep 4, 2014
@anaisbetts anaisbetts deleted the haacked/await-executeasync-race-condition-fix branch September 4, 2014 03:03
@anaisbetts
Copy link
Copy Markdown
Member

✨ Thanks @haacked!

@flagbug
Copy link
Copy Markdown
Contributor

flagbug commented Sep 4, 2014

Wait, I thought

.Finally(() => decrement.Disposable = Disposable.Empty)

and

.Do(
 _ => { },
e => decrement.Disposable = Disposable.Empty,
() => decrement.Disposable = Disposable.Empty) 

are semantically the same? Is my whole Rx knowledge a lie?

@damiensawyer
Copy link
Copy Markdown

Damn I know that feeling!

@jlaanstra
Copy link
Copy Markdown
Contributor

Finally runs after the subscription has been disposed, so it runs after OnCompleted and after OnError.

In the following code the Finally action runs after the actions in the Do.

.Finally(() => decrement.Disposable = Disposable.Empty)
.Do(
 _ => { },
e => decrement.Disposable = Disposable.Empty,
() => decrement.Disposable = Disposable.Empty) 

@flagbug
Copy link
Copy Markdown
Contributor

flagbug commented Sep 4, 2014

Aha! Makes sense, thanks @jlaanstra!

@anaisbetts
Copy link
Copy Markdown
Member

I need to write a blog post about this - basically Finally interacts badly with await, because the Finally block runs shortly after the awaiter returns. In practice, this almost never matters, but in a unit test, it results in very confused people.

Here's the troll scenario that this PR fixes:

bool latestIsExecuting;

someCommand.Subscribe(x => latestIsExecuting = x);
await someCommand.ExecuteAsync();
Assert.False(latestIsExecuting);    // It's true, whyyyyyyyy

@haacked
Copy link
Copy Markdown
Contributor Author

haacked commented Sep 4, 2014

In practice, this almost never matters, but in a unit test, it results in very confused people.

Not necessarily the case. It's hard to predict whether it'll matter or not. But you could imagine a method that has some common logic and it checks to make sure a command isn't currently executing.

You call this method from multiple locations including from the Subscribe callback on the command. In that case, the subscribe callback would always fail because it still thinks the command is executing when in fact it's done.

@lock lock Bot locked and limited conversation to collaborators Jun 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants