HttpClientRequest handle correctly Buffers with a readerIndex > 0

[TanyaGaleyev]

We are using code similar to example. And IndexOutOfBoundsException was observed. After debugging for a while I found a reason of the problem.
In proxy code we have something like:

public void processRequest(HttpServerRequest request) {
  HttpClientRequest destRequest = ...
  request.handler(destRequest::write);
  ...
}

In file HttpClientRequestImpl there is a code:

void write(ByteBuf buf, boolean end) {
  ...
  CompositeByteBuf pending;
  ...
  pending.addComponent(buff).writerIndex(pending.writerIndex() + buff.writerIndex());
  ...
}

And if buff has readerIndex not equal to 0 then call to writerIndex fails with reasonable IndexOutOfBoundsException.
Current workaround is using Buffer.slice() before passing buffer read by http server to http client. But I think that buffers with non 0 readerIndex should be handled properly by HttpClientRequestImpl.

[vietj]

can you provide a reproducer ?

[TanyaGaleyev]

@vietj
The problem is reproduced in unit test in this repo. It turns out that with plain everything works fine with plain HTTP, but starts failing with HTTPS.

[vietj]

thanks

[vietj]

it seems due to this change in Netty : netty/netty@9306333#diff-7191c076eacd4a1a53d60604a5d2a251R219

previous version of duplicate called by HttpClientRequestImpl#write(Buffer) really got a slice and now the duplicate buffer has a readerIndex > 0 because the original duplicated buffer as an adjustment > 0

[TanyaGaleyev]

Thanks for investigation! What are there the next steps for this issue?
Also I wonder why should we assume that buffer coming to HttpClientRequestImpl#write(Buffer) has readerIndex == 0? May be one can use:

pending.addComponent(buff).writerIndex(pending.writerIndex() + buff.READABLEBYTES());

Or even more use CompositeByteBuf.addComponent(boolean increaseWriterIndex, ByteBuf buffer) with increaseWriterIndex == true?
Sorry if my suggestions are silly =)

[vietj]

@TanyaGaleyev actually the buffer we obtain is via io.vertx.core.buffer.Buffer#getByteBuf() that returns a duplicate of the buffer with its readerIndex == 0... until now.

[vietj]

I think it will be fixed in 3.4.2 with improvements in Buffer class and also a test with a buffer that has readerIndex > 0 for write method and a fix.

[vietj]

CompositeByteBuf.addComponent(boolean increaseWriterIndex, ByteBuf buffer) seems like a good fix as you said, we just need to have a corresponding test for it that write a Buffer wrapping a ByteBuf with a readerIndex > 0 which can be obtained with something like Buffer.buffer("foobar").slice(3, 6) I think.

[vietj]

I pushed a fix based on your suggestion with a test, would you mind to check it's fine on your side ?

[TanyaGaleyev]

@vietj
I noticed that there is also another line in HttpClientRequestImpl that can be also affected:

cachedChunks.addComponent(buff).writerIndex(cachedChunks.writerIndex() + buff.writerIndex());

[TanyaGaleyev]

@vietj
I hope to check it in my environment soon. Is there any maven repository where I can take latest dev version?

[vietj]

good catch, I'll try to make a test for this one.

you can get the snapshots from the Sonatype OSS snapshot repository https://oss.sonatype.org/content/repositories/snapshots/

[vietj]

@TanyaGaleyev the computation of the content-length needs also to be fixed headers().set(CONTENT_LENGTH, String.valueOf(buff.writerIndex()));

[vietj]

I fixed those in HttpClientRequest

[TanyaGaleyev]

@vietj
Thanks for fixes. I checked my production code with 3.4.2-SNAPSHOT, everything works fine.

[TanyaGaleyev]

When 3.4.2 will be released?

[vietj]

around May, we aim to start the release process early may and conclude before june

[vietj]

not saying it's a slow process, just saying that we shall do it peacefully :-)

[TanyaGaleyev]

Thanks for info.