Skip to content

Implement isWritable and ensureWritable on ReadOnlyByteBufferBuf#7883

Merged
normanmaurer merged 2 commits into
netty:4.1from
RikkiGibson:readonly_iswritable_fix
Apr 23, 2018
Merged

Implement isWritable and ensureWritable on ReadOnlyByteBufferBuf#7883
normanmaurer merged 2 commits into
netty:4.1from
RikkiGibson:readonly_iswritable_fix

Conversation

@RikkiGibson
Copy link
Copy Markdown
Contributor

Motivation:

It should be possible to write a ReadOnlyByteBufferBuf to a channel without errors. However, ReadOnlyByteBufferBuf does not override isWritable and ensureWritable, which can cause some handlers to mistakenly assume they can write to the ReadOnlyByteBufferBuf, resulting in ReadOnlyBufferException.

Modification:

Added isWritable and ensureWritable method overrides on ReadOnlyByteBufferBuf to indicate that it is never writable. Added tests for these methods.

Result:

Can successfully write ReadOnlyByteBufferBuf to a channel with an SslHandler (or any other handler which may attempt to write to the ByteBuf it receives).

@Test
public void shouldIndicateNotWritable() {
ByteBuf buf = buffer(allocate(8).asReadOnlyBuffer()).clear();
Assert.assertFalse(buf.isWritable());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please also call buf.release() in all tests (just like in the existing tests)

@RikkiGibson
Copy link
Copy Markdown
Contributor Author

@normanmaurer Added try/finally to release the ByteBuf. Let me know if anything else is needed. 👍

@normanmaurer normanmaurer merged commit 1b1f767 into netty:4.1 Apr 23, 2018
@normanmaurer
Copy link
Copy Markdown
Member

@RikkiGibson thanks all good :)

@normanmaurer normanmaurer added this to the 4.1.25.Final milestone Apr 23, 2018
@normanmaurer normanmaurer self-assigned this Apr 23, 2018
@RikkiGibson RikkiGibson deleted the readonly_iswritable_fix branch April 24, 2018 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants