Skip to content
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

ARTEMIS-1482 Add back check for SimpleString #2227

Closed
wants to merge 1 commit into from

Conversation

@mtaylor
Copy link
Contributor

commented Aug 8, 2018

The original fix was removed during a refactor

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

The test that was added in the original PR for ARTEMIS-1482 testOutOfBoundsThrownOnMalformedString seems to still pass on master, as such should this test be updated?

e.g. IndexOutOfBoundsException is thrown correctly

java.lang.IndexOutOfBoundsException: readerIndex(4) + length(100) exceeds writerIndex(4): PooledUnsafeDirectByteBuf(ridx: 4, widx: 4, cap: 5)
at io.netty.buffer.AbstractByteBuf.checkReadableBytes0(AbstractByteBuf.java:1405)

I assume @franz1981 removed it, because he knew the ByteBuf does this check anyhow, so is there another way to get this to occur and not throw IndexOutOfBoundsException, as such maybe a new/updated test case for it?

@franz1981

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

@michaelandrepearce You are right indeed, but the change of @mtaylor is quite smart here: he is avoiding the allocation of byte[] at all, like in the original code 💯

@asfgit asfgit closed this in 70f5512 Aug 8, 2018

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

My point was more ensuring in future its not regressed again.

@mtaylor

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

@michaelandrepearce I agree, if you can come up with an appropriate test it would be beneficial. The reason the test still passed was due to the IOOB exception being moved to a different position in the stack. So, it was thrown but only after the byte array memory was allocated.

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

@mtaylor see #2232 please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.