Release readBuffer when close BufferedReadChannel & BufferedChannel#4030
Release readBuffer when close BufferedReadChannel & BufferedChannel#4030lifepuzzlefun wants to merge 5 commits intoapache:masterfrom
BufferedReadChannel & BufferedChannel#4030Conversation
hangc0276
left a comment
hangc0276
left a comment
There was a problem hiding this comment.
Would you please provide more context about this change?
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
Outdated
Show resolved
Hide resolved
hangc0276
requested review from
StevenLuMT,
dlg99,
eolivelli,
wenbingshen and
zymap
hangc0276
left a comment
hangc0276
left a comment
There was a problem hiding this comment.
I support this change. Would you please add a test to cover this change? Especially cover the readBuffer leak case.
| } | ||
|
|
||
| readBufferStartPosition = Long.MIN_VALUE; | ||
| readBuffer.release(); |
There was a problem hiding this comment.
We'd better use ReferenceCountUtil.release(readBuffer); instead of readBuffer.release();
There was a problem hiding this comment.
Thanks for review, fixed.
| super.close(); | ||
| writeBufferStartPosition.set(0); |
There was a problem hiding this comment.
Should we close and reset the position after setting the closed to true?
| // We set the position of the write buffer of this buffered channel to Long.MAX_VALUE | ||
| // so that there are no overlaps with the write buffer while reading | ||
| fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes()); | ||
|
|
|
|
||
| // BufferedReadChannel is not response for fileChannel close. | ||
|
|
||
| closed = true; |
There was a problem hiding this comment.
Move the closed to the first step?
dlg99
left a comment
dlg99
left a comment
There was a problem hiding this comment.
In general LGTM, but needs a test and more context on scenarios when this is a problem
8 participants
Motivation
Release readBuffer when close
BufferedReadChannel&BufferedChannelBufferedChannel is subclass of BufferedReadChannel, which will create a readBuffer to cache read content.
When close BufferedChannel, the readBuffer should be released to avoid memory leak.