Possible threading issue in FireAsync

[denxorz]

I was trying to figure something out related to async/await, and used a snipped of the stateless code as an example to ask a question on StackOverflow. It seems that the Stateless code might have an threading issue in the FireAsync. The _firing field should be protected by a memory barrier to make sure it is synched after having a continuation, which can happen on a ThreadPool thread.

See this answer for more info: https://stackoverflow.com/a/53649830/2471080

Best regards,
Dennis

[HenningNT]

It is correct the Stateless isn't designed to be thread-safe. There's note in the readme in the section about Async Triggers. The users of Stateless is responsible for assuring thread safety.

[denxorz]

Yes I saw the note, and it makes sense. But the issue here is inside Stateless (so caused by it own implementation), hence the user cannot do anything about this.

[HenningNT]

If adding volatile and switching to concurrentQueue makes it slightly better, than I think I could go ahead and do that.

But Stateless would still not be thread-safe, right?

[HenningNT]

ConcurrentQueue isn't available in netStandard1.0, which Stateless uses. It is available in 1.1 and higher. Would changing to netstandard1.1 (or newer) cause any problems? @JeremyCade @nblumhardt

[nblumhardt]

I think the accepted answer to that question is wrong - await resuming onto a thread pool thread should generate a memory barrier, I believe. Some notes in: https://stackoverflow.com/questions/6581848/memory-barrier-generators

(I could be mistaken, but I think the TPL async/await world would fall apart if this wasn't the case - pretty much every field of every object, local variables in functions, etc. would end up having to be volatile otherwise.)

Nothing to do, here.

[denxorz]

@nblumhardt could you note this issue on the stackoverflow thread as well?

It would be nice if a memory barrier is generated. But if I understand correctly, because of the ContinueAwait(false) you might be working with multiple different contexts and even threads. So it still might be possible that _eventQueue accesses from two threads at the same time.

Edit: Correction; it can be accessed from multiple contexts/threads with ContinueAwait(false). But never simultaneously, so memory barriers should be enough.

[denxorz]

Stephen Cleary also added an answer on the stackoverflow question: https://stackoverflow.com/a/54413147/2471080 .

[nblumhardt]

@denxorz I can't see what Stephen's talking about regarding _eventQueue - for there to be an interleaving like that, multiple threads would need to be active in the state machine concurrently (or memory barriers missing). I could be off track, but personally I'd want to see evidence - a failing load test, perhaps - before adding any additional synchronization, here.

it can be accessed from multiple contexts/threads with ContinueAwait(false). But never simultaneously, so memory barriers should be enough.

I think this sums up my understanding of it, too.

[HenningNT]

I'm closing this, it seems the consensus is that there is no need to change InternalFireQueuedAsync.