Make all write operations atomic

[halter73]

This PR ensures that simultaneous calls to SocketOutput.WriteAsync won't result in a corrupted write.

This was mainly achieved by moving the CopyFrom call inside of SocketOutput.WriteAsync inside of the _contenxtLock. I also moved chunking down a layer inside of SocketOutput, so it could be protected by the same lock.

I tested this change using both the plaintext benchmark and a modified LargeResponseSample that used small chunking, and in both cases the performance was nearly identical <1% to dev.

When I first tried only moving CopyFrom inside of the lock, I saw performance degradation of a few percent. I was able to remedy this by using Monitor.TryEnter to avoid stalling libuv threads.

[cesarblum]

Remove TODO?

[halter73]

This is still a todo though. Someday...

[cesarblum]

What's preventing us from addressing it?

[halter73]

Debugging. All I know is calling WriteAsync on SslStream didn't work when last I tried it. I think the call never completed, but I'm not 100% on that. You are definitely right that I should have left a better comment when I first wrote this code.

[benaadams]

Can you retest post #567 ? Ran into issue #565 with Ssl WriteAsync as its sync.

On the upside; I think I fixed the issue :)

[benaadams]

K, so tests fail changing this to async. Will dig in and do a different PR if I get a resolution. Added issue #569

[halter73]

@benaadams Thanks for looking into this.

[benaadams]

I don't think this is right.

If you write to any stream in System.IO concurrently, you will mashup your data and there is no synchronization applied (e.g. MemoryStream https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/IO/MemoryStream.cs#L540-L613); its up the caller to synchronize their writes - which is normally automatic by waiting for one to complete before doing another.

Also this is making SocketOutput thread-safe for multiple writers but the ISocketOutput presentation is not thread-safe depending on what the filters are doing and not pushing it up to the caller to do the synchronization means the order of events that the filter creates and what the socket does may be mis-matched (e.g. if a filter logs, writes, logs that may cause the socket output to interleave differently to what is logged depending on timings)

[benaadams]

Also a Stream.CopyTo does it in chunks so if that was used the atomicity of the SocketOutput.Write won't help.

[cesarblum]

Why?

[halter73]

We don't want to block libuv threads at all if we can avoid it.

[benaadams]

However... SocketOutput does do leasing blocks and whatnot; so this change might be sensible to protect itself, rather than necessarily providing upstream benefit (or throwing a don't do concurrent writes execption?)

[benaadams]

The question is what's better?

var t1 = file1.CopyToAsync(Body.Response);
var t2 = file2.CopyToAsync(Body.Response);
await Task.WhenAll(t1, t2);

Current - unknown output, unknown state (I'd suggest this is bad)
This change - right byte count output; unknown interleaving, good state
Alternative - exception thrown to user code, good state

[halter73]

@benaadams I was/am also a big proponent for throwing much like we do for ReadAsync.

The filter point you made is interesting, but currently mitigated by the lock I added to SocketOutputStream. The point you make about CopyTo is also a good one, you'll see similar issues if you use JsonSerializer to write to the response streams.

Still, my experience with SignalR has shown me that people will try to write concurrently, and won't see that it's a problem until their app is under stress even if we do throw. And in this case, I think it's important that our internal data structures are in a good sate (i.e. our linked blocks), especially if we can ensure this without much of a perf hit.

[halter73]

Alternative - exception thrown to user code, good state

I like this to. It could very well be achieved with a Interlocked.CompareExchange. I don't think it will have a real impact on perf, but it's nice to tell users that what they're doing is bad. I'll talk to @lodejard about this tomorrow.

[benaadams]

Doing a guard may not cause a throw, so it could still do interleaving as per moving write into lock; but it might be good to have it as an api that can throw on concurrent (public api-wise)? A "be careful" modifier :)

[halter73]

@benaadams I don't think we should remove the other changes just because we decide to throw instead of expand the lock.

Moving the chunking logic down to the SocketOutput layer isn't strictly necessary, but it moves a single logical write down to a lower level which I think is a good thing. I also think rescheduling instead of blocking waiting for locks on the libuv thread is solid. It removes the need for multiple queues just to move work outside of the lock.

What do you think?

[benaadams]

Yeh, think that's good - especially since throwing wasn't the cure all I thought it was going to be :(

[halter73]

So, after further discussion with @lodejard and @CesarBS, I've flip-flopped back to not throwing for concurrent writes. The argument that won the day was that when the would-be-exception was thrown under load it would still only be one of two failure modes. There would still be a potential for out-of-order writes assuming order is actually important, but there is also a small chance you get an exception thrown as well. Also, unlike with concurrent reads, it's plausible that your app could work fine depending on the design even though it writes concurrently. The example given was logging, and how things like Console.Out don't throw for concurrent writes.

If we change our mind (and that's definitely possible), it should be relatively simple to change it to make it throw again.

[benaadams]

👍 makes sense. Rebase time 😝