He-Pin commented on code in PR #1142:
URL: https://github.com/apache/pekko-http/pull/1142#discussion_r3542610110
##########
http-core/src/main/scala/org/apache/pekko/http/impl/util/StreamUtils.scala:
##########
@@ -58,11 +64,14 @@ private[http] object StreamUtils {
override def onPull(): Unit = pull(in)
override def onUpstreamFinish(): Unit = {
+ finished = true
Review Comment:
Setting `finished = true` *before* `finish()` runs means that if `finish()`
itself throws, `postStop()` won't call `cleanup()`. In the current compressor
use case this is harmless because `finishWithBuffer()` already called
`endDeflater()` before anything could throw, and `cleanup()` would be a no-op
anyway. But since `byteStringTransformer` is a general-purpose helper, someone
might later pass a `cleanup` that does more than guard an idempotent resource
release — and then this becomes a subtle leak.
Might be worth either moving `finished = true` to *after* the `finish()`
call (so `postStop` still runs cleanup on failure), or wrapping in try-finally.
Low priority though given the current callers.
##########
http/src/main/scala/org/apache/pekko/http/scaladsl/coding/DeflateCompressor.scala:
##########
@@ -60,10 +60,20 @@ private[coding] class DeflateCompressor private[coding]
(compressionLevel: Int)
protected def finishWithBuffer(buffer: Array[Byte]): ByteString = {
deflater.finish()
val res = drainDeflater(deflater, buffer)
- deflater.end()
+ endDeflater()
res
}
+ private[coding] def endDeflater(): Unit =
+ if (!deflaterEnded) {
+ deflaterEnded = true
+ deflater.end()
+ }
+
+ private[coding] override def cleanup(): Unit = endDeflater()
+
+ private var deflaterEnded = false
Review Comment:
nit: `deflaterEnded` is declared after the methods that reference it (lines
67-73). Works fine in Scala of course, but reads a bit bottom-up. Might be
cleaner to declare it next to `deflater` on line 35 since they're logically
paired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
