Unlink delegate in ReleasableBytesReference once ref count reaches 0 #127058
Conversation
We have some spots where we retain a reference to the `ReleasableBytesReference` instance well beyond its ref-count reaching `0`. If it itself references Netty buffers or `BigArrays` that are not pooled (mostly as a result of overflowing the pooled number of bytes for large messages or under heavy load) then those bytes are not GC-able unless we unlink them here.
original-brownbear
added
>non-issue
:Distributed Coordination/Network
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Makes sense, added an assertion on the delegate into |
| @@ -282,7 +281,7 @@ public void testIndicesPrivilegesAreEnforcedForCcrRestoreSessionActions() throws | |||
| GetCcrRestoreFileChunkAction.REMOTE_TYPE, | |||
| new GetCcrRestoreFileChunkRequest(response2.getNode(), sessionUUID2, leaderIndex2FileName, 1, shardId2) | |||
| ); | |||
| assertThat(getChunkResponse.getChunk().length(), equalTo(1)); | |||
| assertFalse(getChunkResponse.getChunk().hasReferences()); | |||
There was a problem hiding this comment.
Heh this is pretty much exactly what this is about :P this assertion shouldn't have worked but worked as long as we didn't check for having a reference in the length() call. I see no value in the original version and there's a tiny bit of value in checking that we actually released the reference here but I guess we could just as well drop any assertion on the response.
| boolean hasRef = refCounted.hasReferences(); | ||
| // delegate is nulled out when the ref-count reaches zero but only via a plain store, and also we could be racing with a concurrent | ||
| // decRef so need to check #refCounted again in case we run into a non-null delegate but saw a reference before | ||
| assert delegate != null || hasRef == false || refCounted.hasReferences() == false; |
There was a problem hiding this comment.
We could just check refCounted.hasReferences() again, no need to check hasRef right?
There was a problem hiding this comment.
I got this idea into my head that we might be introducing far more happens-before into everything than necessary via assertions and that as a result reproducing some prod bugs isn't happening. But on reflection, it doesn't make too much sense to do so here, a false hasRef read twice should actually never introduce a happens before, I'll clean it up before merging :)
original-brownbear
added
the
auto-backport
|
Thanks David! |
…lastic#127058) We have some spots where we retain a reference to the `ReleasableBytesReference` instance well beyond its ref-count reaching `0`. If it itself references Netty buffers or `BigArrays` that are not pooled (mostly as a result of overflowing the pooled number of bytes for large messages or under heavy load) then those bytes are not GC-able unless we unlink them here.
💚 Backport successful
|
…127058) (#127749) We have some spots where we retain a reference to the `ReleasableBytesReference` instance well beyond its ref-count reaching `0`. If it itself references Netty buffers or `BigArrays` that are not pooled (mostly as a result of overflowing the pooled number of bytes for large messages or under heavy load) then those bytes are not GC-able unless we unlink them here.
We have some spots where we retain a reference to the
ReleasableBytesReferenceinstance well beyond its ref-count reaching0. If it itself references Netty buffers orBigArraysthat are not pooled (mostly as a result of overflowing the pooled number of bytes for large messages or under heavy load) then those bytes are not GC-able unless we unlink them here.