This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
Remove problematic ArrayPool usage #6473
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We decided in the design meeting today that we should only be using ArrayPool in corefx itself in places where we can guarantee the types/methods won't cause corruption, even if due to misuse. Some of the usage we've already put in place doesn't meet this bar and needs to be removed. For example, we use ```ArrayPool<byte>.Shared``` in a special CopyToAsync implementation, but in doing so we hand a buffer from the pool out to the constiuent streams, and a poorly behaving stream could end up holding on to the buffer even after CopyToAsync has returned to the pool, such as if a Stream.WriteAsync implementation had a bug and completed the returned task before it was actually done with the buffer. This commit removes all such usage (which we only very recently added).
|
It makes me sad that these perf gains have to go away, but LGTM |
|
We should consider adding similar APIs that just take an array parameter to be used as the intermediate buffer. |
|
LGTM |
Yes, or the pool to use. Can you open an issue, @KrzysztofCwalina?
Thanks for the reviews. |
stephentoub
added a commit
that referenced
this pull request
Feb 26, 2016
Remove problematic ArrayPool usage
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
…usage Remove problematic ArrayPool usage Commit migrated from dotnet/corefx@9cc4f66
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We decided in the design meeting today that we should only be using ArrayPool in corefx itself in places where we can guarantee the types/methods won't cause corruption, even if due to misuse. Some of the usage we've already put in place doesn't meet this bar and needs to be removed. For example, we use
ArrayPool<byte>.Sharedin a special CopyToAsync implementation, but in doing so we hand a buffer from the pool out to the constiuent streams, and a poorly behaving stream could end up holding on to the buffer even after CopyToAsync has returned to the pool, such as if a Stream.WriteAsync implementation had a bug and completed the returned task before it was actually done with the buffer.This commit removes all such usage (which we only very recently added). There is one usage of
ArrayPool<char>.Sharedleft in corefx, in ZipFile, which meets the bar, as the buffer is never handed out to code outside of the class, is used in a scoped manner on the stack, etc.cc: @sokket, @KrzysztofCwalina, @jkotas
Fies #6472