@stephentoub could you please take a look at the dispose pattern here?

This will end up queueing the clean up of resources such that it could end up happening after Dispose completes. We typically try hard to avoid that, as it means the consumer calling Dispose can't trust that everything has quiesced.

This change is to handle an erroneous case, right? The ManagedWebSocket is being disposed of while there are outstanding operations? And the actual resources involved here are all guarded at some level by SafeHandles, yes? Assuming that's all the case, I'd be inclined to say that Dispose should Dispose of the resources if it's safe to do (e.g. if it can immediately acquire the mutex), but otherwise it should just avoid disposing the resources and let finalization handle it in this erroneous case.

I agree with what you said, however there are cases when calling Dispose is not an erroneous case. Such is an Abort() call triggered for example by a cancellation token backed by a timeout. We could have different implementation for Abort and Dispose I guess, but that will only complicate things.

Another thing is that even if we do what you say, we still need to release the mutex with an asynchronous continuation. If we don't do this, we risk some user code never completing. Here is a simple example of how that might happen:

// User code calls send twice, concurrently in different threads
// Thread 1:
await socket.SendAsync();

// Thread 2:
await socket.SendAsync();

// Thread 1 takes the lock and starts the send operation.
// Before Thread 2 is queued in the AsyncMutex, 
// an Abort() or Dispose() happens in another thread which triggers the new code which 
// tries to acquire the lock and fails and doesn't release the resources, but also doesn't release the lock.

// Thread 2 is queued to wait

In this case since, because we don't release the AsyncMutex, Thread 2 code will never complete.

This also can be avoided, if we implement TryLock method on the AsyncMutex so we can have a fully synchronous path for lock / unlock.

If you decide we should go down that path, I would advise that we implement the disposal of the unmanaged resources in the Send / Receive methods when they exit - we can check if _disposed is true. The reason for this is that usually the websocket without compression takes about 500 bytes of memory. Using compression this increases up ot 500KB depending on the window size. I am sure that there is user code out there that doesn't release strong references to websockets early, this would incur a tremendous penalty on the memory used by an application. I am even sure that there is code out there leaking websockets, and 500 bytes is not very much but 500KB is.

This can even be DDOS'ed by having many websockets connect to an endpoint and have then behave in a such a way to trigger Aborts by the server side.

My point being is that this implementation, in the end, will have the same net result as the current code, but the current code is far simpler to understand.

This also can be avoided, if we implement TryLock method on the AsyncMutex so we can have a fully synchronous path for lock / unlock.

Right, that's what I had in mind.

But taking a step back, I'm not clear on how we end up in this situation in the first place. Disposing of anything shouldn't lead to AVs... that's one of the main purposes of SafeHandle, where it defers releasing the underlying handle while a resource is still in use. Are we missing DangerousAddRef/Release calls around unsafe use of the underlying handle?

The issue here is not the SafeHandle, it's that it is being disposed while being used by another thread to inflate / deflate.

Edit: Actually, let me take a look, I misunderstood what you meant.

@stephentoub You were correct. I've pushed a new implementation which uses DangerousAddRef/Release to tackle the issue. Let me know what you think.

Should we double-check for null here and below?

It is not needed, because the resource can never become null. We only need to check once at the beginning of the method.

Ah, I see, that's what you removed in the first place, right? once assigned, it will now stay assigned