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.

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

ByteBufs which are not resizable should not throw in ensureWritable(i… #7004

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@Scottmitch
Member

Scottmitch commented Jul 21, 2017

…nt,boolean)

Motivation:
ByteBuf#ensureWritable(int,boolean) returns an int indicating the status of the resize operation. For buffers that are unmodifiable or cannot be resized this method shouldn't throw but just return 1.
ByteBuf#ensureWriteable(int) should throw unmodifiable buffers.

Modifications:

  • ReadOnlyByteBuf should be updated as described above.
  • Add a unit test to SslHandler which verifies the read only buffer can be tolerated in the aggregation algorithm.

Result:
Fixes #7002.

@Scottmitch Scottmitch added the defect label Jul 21, 2017

@Scottmitch Scottmitch added this to the 4.0.50.Final milestone Jul 21, 2017

@Scottmitch Scottmitch self-assigned this Jul 21, 2017

@Scottmitch Scottmitch requested a review from normanmaurer Jul 21, 2017

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Jul 21, 2017

@ejona86 - PTAL.

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Jul 21, 2017

Contributor

The core change LGTM.

Contributor

ejona86 commented Jul 21, 2017

The core change LGTM.

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 22, 2017

Member

@normanmaurer - copy/paste got outa control in the unit tests ... issues should hopefully be fixed ... PTAL.

Member

Scottmitch commented Jul 22, 2017

@normanmaurer - copy/paste got outa control in the unit tests ... issues should hopefully be fixed ... PTAL.

public void ensureWritableWithEnoughSpaceShouldNotThrow() {
ByteBuf buf = newBuffer(1, 10);
buf.ensureWritable(3);
assertThat(buf.writableBytes(), is(greaterThanOrEqualTo(3)));

This comment has been minimized.

@normanmaurer

normanmaurer Jul 22, 2017

Member

missing buf.release()

@normanmaurer

normanmaurer Jul 22, 2017

Member

missing buf.release()

This comment has been minimized.

@normanmaurer

normanmaurer Jul 22, 2017

Member

I guess this is also the reason why the build failed with a leak detected.

@normanmaurer

normanmaurer Jul 22, 2017

Member

I guess this is also the reason why the build failed with a leak detected.

Show outdated Hide outdated buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
Show outdated Hide outdated buffer/src/test/java/io/netty/buffer/SlicedByteBufTest.java
ByteBufs which are not resizable should not throw in ensureWritable(i…
…nt,boolean)

Motivation:
ByteBuf#ensureWritable(int,boolean) returns an int indicating the status of the resize operation. For buffers that are unmodifiable or cannot be resized this method shouldn't throw but just return 1.
ByteBuf#ensureWriteable(int) should throw unmodifiable buffers.

Modifications:
- ReadOnlyByteBuf should be updated as described above.
- Add a unit test to SslHandler which verifies the read only buffer can be tolerated in the aggregation algorithm.

Result:
Fixes #7002.
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch
Member

Scottmitch commented Jul 22, 2017

@normanmaurer - PTAL

public void ensureWritableWithEnoughSpaceShouldNotThrow() {
ByteBuf slice = newBuffer(10);
ByteBuf unwrapped = slice.unwrap();
slice.unwrap().writerIndex(unwrapped.writerIndex() + 5);

This comment has been minimized.

@normanmaurer

normanmaurer Jul 22, 2017

Member

nit: i think you could also just use `unwrapped .´ here ?

@normanmaurer

normanmaurer Jul 22, 2017

Member

nit: i think you could also just use `unwrapped .´ here ?

This comment has been minimized.

@Scottmitch

Scottmitch Jul 22, 2017

Member

fixed

@Scottmitch
public void ensureWritableWithNotEnoughSpaceShouldThrow() {
ByteBuf slice = newBuffer(10);
ByteBuf unwrapped = slice.unwrap();
slice.unwrap().writerIndex(unwrapped.writerIndex() + 5);

This comment has been minimized.

@normanmaurer

normanmaurer Jul 22, 2017

Member

nit: I think you could just used unwrapped. here ?

@normanmaurer

normanmaurer Jul 22, 2017

Member

nit: I think you could just used unwrapped. here ?

This comment has been minimized.

@Scottmitch

Scottmitch Jul 22, 2017

Member

fixed

@Scottmitch
@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jul 22, 2017

Member

4.1 (452fd36) 4.0 (0090c9c)

Member

Scottmitch commented Jul 22, 2017

4.1 (452fd36) 4.0 (0090c9c)

@Scottmitch Scottmitch closed this Jul 22, 2017

@Scottmitch Scottmitch deleted the Scottmitch:readonly_slice_ensurewritable branch Jul 22, 2017

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