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

[NOT FOR MERGE] test with 1.1.0-M1-RC1 #548

Closed
wants to merge 4 commits into from

Conversation

pjfanning
Copy link
Contributor

testing the RC

@pjfanning pjfanning marked this pull request as draft May 10, 2024 11:44
@pjfanning pjfanning force-pushed the 1.1.0-dep branch 2 times, most recently from a8e0bec to ccfb346 Compare May 10, 2024 15:04
@pjfanning
Copy link
Contributor Author

pjfanning commented May 10, 2024

I brought back the change that was in #539 and found that HeaderDecompression only works with ByteArrayInputStream. The legacy code always converted the ByteString to ByteArrayInputStream. The PR539 code can return a SequenceInputStream for byte strings that are made up of multiple smaller byte strings.

I tried the failing tests and the InputStreams have the same contents whether we force the use of ByteArrayInputStream or use the new ByteString.asInputStream code that can return ByteArrayInputStream or SequenceInputStream.

I think the issue is in the hpack (Twitter lib). Its Decoder uses available calls to see if there are more bytes. I have found that available is not reliable in all InputStream implementations. Most of the time, you just call read and stop reading when you get a -1 result.

https://github.com/twitter/hpack/blob/master/hpack/src/main/java/com/twitter/hpack/Decoder.java

If you look at SequenceInputStream, you'll see that available() can return 0 when you have read through one of its underlying InputStreams but haven't yet moved on the next stream.

@@ -93,7 +93,7 @@ private[http2] final class HeaderDecompression(masterHeaderParser: HttpHeaderPar
}
}
try {
decoder.decode(ByteStringInputStream(payload), Receiver)
decoder.decode(ByteStringInputStream.asByteArrayInputStream(payload), Receiver)
Copy link
Member

Choose a reason for hiding this comment

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

Cool!

@pjfanning pjfanning closed this May 21, 2024
@pjfanning pjfanning deleted the 1.1.0-dep branch May 21, 2024 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants