Fix broken encoding when publisher has no content #1345
Conversation
tkountis
force-pushed
the
fix/broken-encoding-empty-pub
branch
from
8be98fa to
d29f7a2
Compare
|
test failure attributed to #999 |
servicetalk-http-api/src/main/java/io/servicetalk/http/api/ContentCodingHttpServiceFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/ContentCodingHttpServiceFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-http-api/src/main/java/io/servicetalk/http/api/ContentCodingHttpServiceFilter.java
Outdated
Show resolved
Hide resolved
servicetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/AbstractZipContentCodec.java
Outdated
Show resolved
Hide resolved
...lk-http-netty/src/test/java/io/servicetalk/http/netty/ServiceTalkEmptyContentCodingTest.java
Outdated
Show resolved
Hide resolved
servicetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/AbstractZipContentCodec.java
Outdated
Show resolved
Hide resolved
|
another test failure attributed to #999 |
tkountis
force-pushed
the
fix/broken-encoding-empty-pub
branch
from
a1a0194 to
ef971bc
Compare
tkountis
changed the title
servicetalk-encoding-api/src/main/java/io/servicetalk/encoding/api/AbstractZipContentCodec.java
Outdated
Show resolved
Hide resolved
|
|
||
| if (!headerWritten) { | ||
| // This will produce header bytes | ||
| output = newDeflaterOutputStream(stream); |
There was a problem hiding this comment.
can output be created up front too? I know this is Closable, but the existing approach doesn't account for Subscription.cancel() anyways and I would propose we revisit lifetime in a followup PR (for better isolation of changes).
There was a problem hiding this comment.
If output is created up front, then we need to allocate up-front too because it starts writing the header bytes immediately. This way we only allocate when we start consuming. I am re-working the overall lifetime + enhancements in a follow PR, let's keep further changes out of this one. I can revert if you prefer the old way, but I think this follows the 'cold' semantics of ST.
| @@ -589,10 +571,10 @@ private void verifyCrc(Buffer in) throws IOException { | |||
| } | |||
|
|
|||
| static class SwappableBufferOutputStream extends OutputStream { | |||
| @Nullable | |||
There was a problem hiding this comment.
if you don't wan't to worry about null you can initialize to EmptyBuffer.EMPTY_BUFFER
There was a problem hiding this comment.
I did consider that, but spotbugs starts complaining with RV_RETURN_VALUE_IGNORED_NO_SIDE_EFFECT: Return value of method without side effect is ignored and I didn't won't to suppress it.
There was a problem hiding this comment.
hum this seems like a general problem that we may want to address, but independent issue
Motivation:
When a payload publisher on the response flow is not emitting any items, the encoder will not emit the header, thus a client will not be able to decode the response.
Modifications:
Make sure the header is emitted even for empty publishers.
Extra checks where added to skip encoding & headers when the request doesn't support body-messages.
Result:
Clients will be able to decode correctly encoded response payload, even when empty.