http2: remove single use ByteStringInputStream #833
Conversation
| val bis = ByteStringInputStream(payload) | ||
| try { | ||
| decoder.decode(bis, Receiver) | ||
| decoder.decode(payload.compact.asInputStream, Receiver) // only compact ByteString supports InputStream with mark/reset |
There was a problem hiding this comment.
I'm not against this generally. One edge case is a ByteString that has a backing array but a non-zero offset.
We can get a safe InputStream directly from those ByteStreams without compacting them (an InputStream that supports mark/reset). I think in practice such ByteStrings are rare. So if we agree that such ByteStrings are not worrying about then I'm happy enough to go with this change.
pjfanning
left a comment
pjfanning
left a comment
There was a problem hiding this comment.
lgtm - I will merge if there are no objections
There was a problem hiding this comment.
Pull request overview
This PR simplifies the HTTP/2 HPACK header decompression code by removing a single-use utility class ByteStringInputStream and replacing its usage with the direct ByteString API method compact.asInputStream. The PR also removes unnecessary try/finally blocks around InputStream usage since array-backed InputStreams don't hold system resources that need explicit cleanup.
- Removes the custom
ByteStringInputStreamutility class (36 lines) - Replaces
ByteStringInputStream(bytes)calls withbytes.compact.asInputStream - Removes unnecessary try/finally blocks and nesting around decoder calls
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/ByteStringInputStream.scala | Deleted utility class that wrapped ByteString as InputStream - functionality replaced by built-in ByteString API |
| http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/hpack/HeaderDecompression.scala | Updated to use payload.compact.asInputStream directly, removed unnecessary try/finally block, added clarifying comment about mark/reset support |
| http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2FrameHpackSupport.scala | Updated to use bytes.compact.asInputStream directly, removed unnecessary try/finally block and import statement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Also remove useless nesting / finally blocks, arrays do not need closing. Fixes apache#553
pjfanning
force-pushed
the
remove-ByteStringInputStream
branch
from
5da1cf9 to
096c231
Compare
3 participants
Also remove useless nesting / finally blocks, arrays do not need closing.
Fixes #553
Simpler variant of #820. WDYT, @pjfanning ?