-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Conversation
if (_charBytes != null) | ||
ArrayPool<byte>.Shared.Return(_charBytes); | ||
if (_charBuffer != null) | ||
ArrayPool<char>.Shared.Return(_charBuffer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to return the buffer before calling dispose on the stream? This would ensure the buffers are returned even if stream.Dispose throws.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this end up returning these buffers twice if it's Dispose'd twice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the code should guard against double return either by checking _isDisposed, or setting _buffer to null after the first clear. Also, this should be thread-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
Thanks for working on this. |
@sokket, before doing further work on this, see the discussion here: #5954 (comment) |
cc: @jkotas |
@stephentoub I didn't see a conclusion to that thread...should I back out this and the compression changes? |
@sokket, thanks for doing this. Based on the discussion at the design meeting today, I'm closing this. In cases where these types are misused, we can't guarantee in these cases that we won't continue using the buffer even after it's been returned, e.g. BinaryReader passes its _buffer to the underlying Stream, which may be buggy and hold onto and use that byte[] well after it's been returned to the pool. |
Starting to buffer all the things from #5598; adding Buffer Pooling to the BufferedStream, BinaryWriter, BinaryReader, and Stream classes.
Thanks to @ericstj for helping get the project.json and csproj files correct
@stephentoub @weshaggard