Redesigning PoolReturnRead #559
Comments
|
Sounds good overall.
Doesn't this mean that we could have garbage data after the body? How do we "guarantee" the |
|
When we return the Stream to the pool, we can check that the |
jsha
added a commit
that referenced
this issue
Dec 4, 2022
Introduce PoolReturner, a handle on an agent and a PoolKey that is capable of returning a Stream to a Pool. Make Streams keep track of their own PoolReturner, instead of having PoolReturnRead keep track of that information. For the LimitedRead code path, get rid of PoolReturnRead. Instead, LimitedRead is responsible for returning its Stream to the Pool after its second-to-last read. In other words, LimitedRead will return the stream if the next read is guaranteed to return Ok(0). Constructing a LimitedRead of size 0 is always wrong, because we could always just return the stream immediately. Change the size argument to NonZeroUsize to enforce that. Remove the Done trait, which was only used for LimitedRead. It was used to try and make sure we returned the stream to the pool on exact reads, but was not reliable. This does not yet move the ChunkDecoder code path away from PoolReturnRead. That requires a little more work. Part 1 of #559. Fixes #555.
This was referenced Dec 7, 2022
Merged
|
Closing since we're moving to ureq 3.x. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
PoolReturnRead is important because we need to be able to hand something back to the user that (a) implements Read and (b) returns the Stream to the Pool as soon as it is correct to do so. But it relies on a property of the Read interface that is not actually guaranteed and in fact is often not true. It relies on upstream Readers always reading until they get to
Ok(0). We tried to fix this with theDonetrait, for the LimitedRead case. But it's come up again in #549 and #555, which makes me think we are using the wrong abstraction.PoolReturnRead only ever wraps two types:
LimitedReadandChunkDecoder. We can move the "return to pool" logic down a level and make those two types responsible for returning their Stream when it is viable. For instance,LimitedReadcould have anOption<R>, and so long as it has bytes left to read and thatOptionisSome, it would read from the inner reader. If any read would leave zero bytes left, it wouldtake()the Option and return it to the pool before returning fromread(). ForChunkedDecoderwe would need aBufReadas the inner reader, so that it could look ahead and see if the next bytes are the end-of-stream indicator, and do the same thing.The end result of these would be that in almost all cases, the stream would be returned to the pool on the second-to-last read (the one returning
Ok(n)), not the last read (returningOk(0)). And if the user never calls a read that returnsOk(0), we don't have to care.The text was updated successfully, but these errors were encountered: