Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netty: Eliminate buffer release when there is an error as caller still owns the buffer. #10537

Merged
merged 6 commits into from
Sep 15, 2023
Merged

netty: Eliminate buffer release when there is an error as caller still owns the buffer. #10537

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
80df75d
netty: Eliminate buffer release when there is an error as caller stil…
larry-safran Sep 5, 2023
426f1e6
Move resetting the composite reader index to before the release as it…
larry-safran Sep 6, 2023
6d97e70
Update netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
larry-safran Sep 14, 2023
4696ad5
Move release in tests from finally to catch block.
larry-safran Sep 14, 2023
2d4f1a2
Merge remote-tracking branch 'refs/remotes/origin/is10255' into is10255
larry-safran Sep 14, 2023
9a46d9e
Move release back to finally block in tests, but skip it if the merge…
larry-safran Sep 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 1 addition & 8 deletions netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,16 +204,9 @@ static void mergeWithCompositeTail(
composite.addFlattenedComponents(true, newTail);
// New tail's ownership transferred to the composite buf.
newTail = null;
in.release();
in = null;
// Restore the reader. In case it fails we restore the reader after releasing/forgetting
// the input and the new tail so that finally block can handles them properly.
composite.readerIndex(prevReader);
in.release(); // Successful so took over ownership of in
larry-safran marked this conversation as resolved.
Show resolved Hide resolved
} finally {
// Input buffer was merged with the tail.
if (in != null) {
in.release();
}
// If new tail's ownership isn't transferred to the composite buf.
// Release it to prevent a leak.
if (newTail != null) {
Expand Down
10 changes: 6 additions & 4 deletions netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,8 +536,8 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
fail("Cumulator didn't throw");
} catch (UnsupportedOperationException actualError) {
assertSame(expectedError, actualError);
// Input must be released unless its ownership has been to the composite cumulation.
assertEquals(0, in.refCnt());
// Because of error, ownership shouldn't have changed so should not have been released.
assertEquals(1, in.refCnt());
// Tail released
assertEquals(0, tail.refCnt());
// Composite cumulation is retained
Expand All @@ -546,6 +546,7 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
assertEquals(0, compositeThrows.numComponents());
} finally {
compositeThrows.release();
in.release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep it abovefail("Cumulator didn't throw");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

Expand All @@ -572,8 +573,8 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
fail("Cumulator didn't throw");
} catch (UnsupportedOperationException actualError) {
assertSame(expectedError, actualError);
// Input must be released unless its ownership has been to the composite cumulation.
assertEquals(0, in.refCnt());
// Because of error, ownership shouldn't have changed so should not have been released.
assertEquals(1, in.refCnt());
// New buffer released
assertEquals(0, newTail.refCnt());
// Composite cumulation is retained
Expand All @@ -582,6 +583,7 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
assertEquals(0, compositeRo.numComponents());
} finally {
compositeRo.release();
in.release();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should keep it abovefail("Cumulator didn't throw");

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to release when there is an error since the new paradigm is that only successes change ownership.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed IRL, we do want to move it, but to another place

}
}
}
Expand Down