RATIS-2426. Fix memory leak in ServerRequestStreamObserver#1368
RATIS-2426. Fix memory leak in ServerRequestStreamObserver#1368ivandika3 wants to merge 14 commits into
Conversation
ivandika3
changed the title
Add a static counter in GrpcServerProtocolService that tracks handleError closures where previousOnNext was not cleaned up. The test asserts the counter remains zero, which fails without the previousOnNext.set(null) fix. Made-with: Cursor
szetszwo
left a comment
szetszwo
left a comment
There was a problem hiding this comment.
@ivandika3 , thanks for working on this! Please see the comment inlined. (Sorry that I missed this PR earlier.)
| if (event == Event.COMPLETE) { | ||
| onCompleted(); | ||
| } else { | ||
| cancelStream("stop due to " + event); |
There was a problem hiding this comment.
We have the following call sequence:
- AppendLogResponseHandler.onError(..) / onCompleted() -> resetClient(..) -> stop()
In the gRPC example,
- onCompleted() implementation will response onCompleted() but
- onError(Throwable t) only calls logger.log(Level.WARNING, ...)
So, I think stop() should not call cancelStream(..)
| */ | ||
| @ParameterizedTest | ||
| @MethodSource("data") | ||
| public void testLogAppenderStreamCleanupOnRestart(Boolean separateHeartbeat) throws Exception { |
There was a problem hiding this comment.
I tried to revert GrpcLogAppender and GrpcServerProtocolService (except for GRPC_SERVER_HANDLE_ERROR). Both testLogAppenderStreamCleanupOnRestart and testFollowerHandleErrorCleanup can pass. So, they probably cannot test the code change.
|
@szetszwo Thanks for the review. I'm still not 100% sure about this fix yet. I'll revisit this when I have time. |
|
This PR has been marked as stale due to 60 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in ~30 days. |
2 participants
What changes were proposed in this pull request?
Please refer to https://issues.apache.org/jira/browse/RATIS-2426
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-2426
How was this patch tested?
UT