Return shared ArrayPool #459
Conversation
shargon
requested review from
joseharriaga,
trrwilson and
ShivangiReja
as code owners
There was a problem hiding this comment.
Hi @shargon. Thank you for your contribution and interest in improving the OpenAI developer experience. Thanks for catching these, as these appear to be legitimate leaks to me.
@joseharriaga: This looks to be a clean fix that is safe to make here. Can you please confirm and, if so, complete the merge after the requested pattern change?
jsquire
dismissed
their stale review
Intended to just add a comment, as a pattern adjustment was requested.
There was a problem hiding this comment.
Thanks, @shargon! I touched base with @joseharriaga earlier and confirmed that this won't cause issues with in-flight work upstream. Merging.
| @@ -26,6 +26,7 @@ public AsyncWebsocketMessageResultEnumerator(WebSocket webSocket, CancellationTo | |||
|
|
|||
| public ValueTask DisposeAsync() | |||
| { | |||
| ArrayPool<byte>.Shared.Return(_receiveBuffer); | |||
There was a problem hiding this comment.
If disposal is performed multiple times, this will end up returning the buffer multiple times. That needs to be fixed, assuming this enumerator can be returned out to user code.
There was a problem hiding this comment.
This one is on me, as I was under the impression that it was safe to return multiple times. However, looks like I'm incorrect.
There was a problem hiding this comment.
This use of ArrayPool looks dangerous in general. The enumerator is yielding references to this array. If the consumer holds on to those yielded elements after disposal and reads from them (which is technically fine for them to do), that's a use-after-free bug.
I think the right answer here is to stop using ArrayPool here, i.e. revert the addition of the Return and change the Rent to just be a normal allocation. That won't be any worse than the state of allocation today because the array was already not being returned. If in the future perf problems demonstrate pooling is important, it can be re-evaluated holistically.
There was a problem hiding this comment.
Actually, it may not be as bad as I feared... tracing through where _recieveBuffer goes, it looks like it may not actually be stored into the yielded object but instead just having its data copied out? If so, using an ArrayPool array would be ok. But we still want to ensure it's only returned to the pool once.
There was a problem hiding this comment.
Filed #474 for investigation.
| } | ||
| finally | ||
| { | ||
| ArrayPool<byte>.Shared.Return(bytes); |
There was a problem hiding this comment.
This is OK. FWIW, though, we generally don't consider it a bug to not return an ArrayPool array in exceptional situations.
Rent byte arrays should be returned