What is the purpose of having this be a stream and not just some helper function? Is this CopyToAsyncStream getting returned somewhere that I don't see?

It's getting passed as the destination argument to the wrapped Stream's CopyToAsync:
https://github.com/dotnet/corefx/pull/13184/files#diff-35ce8d1b7a1f37dccabcc9a644648242R738

Where is this being called? The only write I see is on the destination stream on line 732.

This instance gets passed as the destination argument to the wrapped stream's CopyToAsync, so that wrapped stream's CopyToAsync pushes data to this instance via this WriteAsync. That's why this is a Stream ;)

nvm, copyTo on 738

Beat me by 8 seconds on the response :)

😄

Instead of breaking here, we should first check if _deflateStream._inflater.Finished == falselike we do in ReadAsync to handle the case where bytesRead == 0 but the inflation is not yet complete. This can happen in the rare case where the header is the exact amount of bytes as the inflation buffer.

Interesting. Ok, I'll fix that. Thanks.

Spoke about this offline with@ianhays; we think it's fine as-is, but he's putting a test case together.

I understand the reasoning here but I'm not sure it's what a user expects.

I guess the question is, if I specify a buffer of size N, do I expect that N is a bound on the memory used, or do I expect that the copy happens in chunks of N (when possible), regardless of the memory this would use?

Another thing to note is that in the CopyTo ArrayPool helper we were using before this PR we did not do any halving of the bufferSize, so we did a ReadAsync(..bufferSize) followed by a WriteAsync(...bufferSize).

Personally I would expect a copyToAsync(bufferSize 10) to complete in one Read/Write call when there is only 10 bytes in the source stream.

Fair question. My primary concern is around the case where the buffer pool is exhausted. If there are plenty of available buffers in the pool, then using 2 instead of 1 at the requested size isn't that big a deal. But if there aren't any buffers in the pool, we're going to now end up allocating 2 buffers where we previously would have only allocated 1. I figured we could at least allocate no more in total size than the user requested in such cases.

I don't have enough data to really argue one way or the other, though. If you think I should remove this and just go with the original bufferSize, I can.

Personally I would expect a copyToAsync(bufferSize 10) to complete in one Read/Write call when there is only 10 bytes in the source stream.

Given that DeflateStream changes the amount of data as part of the transformation, you generally can't expect 10 bytes of input with a 10 byte buffer will result in a single write to the output stream. But I get the general point.

I'll remove the halving. We can add it later if this looks like an issue.

Very minor nit: seems like it would be better to rent the ArrayPool in CopyToAsyncStream; that way it's more cleanly encapsulated if we ever wanted to change to a different buffer pool implementation.

Ok. We can rent it in the CopyToAsyncStream ctor, so I'll change it to do that. If we instead rent it in CopyFromSourceToDestination, which would lead to the best symmetry/encapsulation, we'd unfortunately pay a bit more for it, as we'd end up spending a field for each of the bufferSize and the rented byte[].

Couldn't you pass bufferSize to CopyFromSourceToDestination?

Couldn't you pass bufferSize to CopyFromSourceToDestination?

CopyFromSourceToDestination is an async method. Any arguments to that method will end up on its state machine object.

Ah, good point.

I do wonder why this isn't an exception, if we're going to specifically check for it. (Or if we only want to check for this in debug, then the count should be debug-only too.)

I wonder that as well, but I didn't want to mess with that as part of this change; I just encapsulated the check into its own method so that we can more easily do such things in the future (it also helps workaround a compiler warning regarding passing a ref on a MarshalByRefObject). There are earlier checks for whether _asyncOperations != 0, throwing an exception in such cases... I don't know why those checks aren't just based on these atomic operations so as to avoid race conditions, but such a change now would be a small breaking change, as it could impact which exceptions get thrown in a very niche corner cases. Probably not something we should care too much about, but definitely something to be left for later.