Skip to content

fix: resource leak when JournalChannel is not fully initialized#4340

Merged
eolivelli merged 1 commit into
apache:masterfrom
hezhangjian:journal-channel-close
May 9, 2024
Merged

fix: resource leak when JournalChannel is not fully initialized#4340
eolivelli merged 1 commit into
apache:masterfrom
hezhangjian:journal-channel-close

Conversation

@hezhangjian

@hezhangjian hezhangjian commented May 5, 2024

Copy link
Copy Markdown
Member

Motivation

There is a resource leak in the JournalChannel implementation of Apache Bookkeeper when the channel is closed before it is fully initialized (specifically, before the writing of headers begins). In such cases, the buffer channel (bc) is not yet instantiated, but the file channel (fc) might already be open.

Changes

Updated the close() method in JournalChannel to include a conditional check that closes the file channel (fc) if the buffer channel (bc) is null. This ensures that all resources are properly released, even if the JournalChannel is prematurely closed before full initialization.

wrie header method

private void writeHeader(Journal.BufferedChannelBuilder bcBuilder,
int writeBufferSize) throws IOException {
int headerSize = (V4 == formatVersion) ? VERSION_HEADER_SIZE : HEADER_SIZE;
ByteBuffer bb = ByteBuffer.allocate(headerSize);
ZeroBuffer.put(bb);
bb.clear();
bb.put(magicWord);
bb.putInt(formatVersion);
bb.clear();
fc.write(bb);
bc = bcBuilder.create(fc, writeBufferSize);
forceWrite(true);
nextPrealloc = this.preAllocSize;
fc.write(zeros, nextPrealloc - journalAlignSize);
}

@hezhangjian
hezhangjian force-pushed the journal-channel-close branch from 1e0fe59 to 9c9a669 Compare May 6, 2024 06:41
Signed-off-by: ZhangJian He <shoothzj@gmail.com>
@hezhangjian
hezhangjian force-pushed the journal-channel-close branch from 9c9a669 to 879e2c1 Compare May 8, 2024 05:29
@hezhangjian

Copy link
Copy Markdown
Member Author

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants