netty: Eliminate buffer release when there is an error as caller still owns the buffer. #10537
Conversation
…l owns the buffer.
There was a problem hiding this comment.
I don't think this is right. I was very thorough working on this code. This change contradicts the comments in the test:
// Input must be released unless its ownership has been to the composite cumulation.
assertEquals(1, in.refCnt());Not releasing input buffer in finally is dangerous.
I need more time to look at the reported issue and understand the consequences of this change.
… that fails then the operation hasn't completed correctly and the ownership of the buffer should remain with the caller.
|
@sergiitk after our discussion yesterday are you now okay with this? |
| @@ -582,6 +583,7 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex, | |||
| assertEquals(0, compositeRo.numComponents()); | |||
| } finally { | |||
| compositeRo.release(); | |||
| in.release(); | |||
There was a problem hiding this comment.
I think we should keep it abovefail("Cumulator didn't throw");
There was a problem hiding this comment.
We need to release when there is an error since the new paradigm is that only successes change ownership.
There was a problem hiding this comment.
Discussed IRL, we do want to move it, but to another place
| @@ -546,6 +546,7 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex, | |||
| assertEquals(0, compositeThrows.numComponents()); | |||
| } finally { | |||
| compositeThrows.release(); | |||
| in.release(); | |||
There was a problem hiding this comment.
I think we should keep it abovefail("Cumulator didn't throw");
There was a problem hiding this comment.
It is expected the in the case of an error that the ownership remained with the calling method so it is this method's responsibility to do the release.
Co-authored-by: Sergii Tkachenko <hi@sergii.org>
…l owns the buffer. (grpc#10537) * netty: Eliminate buffer release when there is an error as caller still owns the buffer. --------- Co-authored-by: Sergii Tkachenko <hi@sergii.org>
Fixes #10255