Codecov Report

Merging #1797 into master will decrease coverage by 0.02%.
The diff coverage is 70%.

@@             Coverage Diff              @@
##             master    #1797      +/-   ##
============================================
- Coverage     72.85%   72.83%   -0.03%     
- Complexity     8636     8687      +51     
============================================
  Files           770      772       +2     
  Lines         34016    34145     +129     
  Branches       4185     4196      +11     
============================================
+ Hits          24781    24868      +87     
- Misses         7105     7131      +26     
- Partials       2130     2146      +16

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 939ddd2...9f26c17. Read the comment docs.

I wonder if it's a good idea to remove the overloaded methods that accept offset and length or not. It introduces ambiguity that forces a user to guess if the specified array will be copied or not, because no copy may be made if offset is 0 and length is same with the length of the array. However, it can be convenient in cases where you want to avoid unnecessary array copy:

if (offset == 0 && length == data.length) {
    return HttpData.of(data);
} else {
    return HttpData.of(Arrays.copyOfRange(data, offset, offset + length));
}

Perhaps we could rename all factory methods of HttpData for clarity:

• of(byte[]) and of(ByteBuf) to wrap() or wrapperOf()
• of(byte[], int offset, int length) to wrapOrCopy(byte[], int, int) or wrapperOrCopyOf(...)

Thoughts?

Yeah I was definitely thinking of renaming of(byte[]) to wrap, seems like a good idea.

wrapOrCopy seems confusing since having a method that may reflect mutations in the input or may not seems unpredictable. In that case I would consider having copyOf(byte[]), copyOf(byte[], int, int) and wrap(byte[], int, int) which is implemented as return new ByteBufHttpData(Unpooled.wrappedBuffer(bytes, offset, size), false). If we expose these on Aggregated* too though it'd be a whole lot of APIs.

wrapOrCopy seems confusing since having a method that may reflect mutations in the input or may not seems unpredictable. In that case I would consider having copyOf(byte[]), copyOf(byte[], int, int) and wrap(byte[], int, int) which is implemented as return new ByteBufHttpData(Unpooled.wrappedBuffer(bytes, offset, size), false).

+1

If we expose these on Aggregated* too though it'd be a whole lot of APIs.

Agreed. Let's leave them removed in Aggregated* and let users use HttpData when they want to slice the array.

Tried changing to the proposed API let me know if it looks ok

Also I'm wondering if the ByteBuf versions should be prefixed with unsafe or be in a different package

Pressed a wrong button. I actually requested changes. 😉

I found a gotcha with wrap(byte[], int, int) when implemented using an unpooled bytebuffer - previously it was possible to publish the same HttpData created that way multiple times, but now it isn't since the first subscriber would read all the bytes. Do we want to support this?

I found a gotcha with wrap(byte[], int, int) when implemented using an unpooled bytebuffer - previously it was possible to publish the same HttpData created that way multiple times, but now it isn't since the first subscriber would read all the bytes. Do we want to support this?

That's a good point. Perhaps we should introduce another HttpData implementation dedicated to wrap(byte[], int, int) which does not involve a ByteBuf?

Tried adding a separate implementation, how does it look?

@anuraaga Looks good. Left just some comments.

Thanks, @anuraaga!