Closed stream error when using GZip on 1.3.0.dev

[jwoertink]

This code works on Crystal 1.2.1 but throws an IO::Error on 1.3.0.dev

require "http"
require "compress/gzip"

output = IO::Memory.new
request = HTTP::Request.new("GET", "/")
response = HTTP::Server::Response.new(output)
context = HTTP::Server::Context.new(request, response)
context.request.headers["Accept-Encoding"] = "gzip"

context.response.content_type = "text/html"
context.response.status_code = 200
context.response.headers["Content-Encoding"] = "gzip"
context.response.output = Compress::Gzip::Writer.new(context.response.output, sync_close: true)
context.response.print "some body"

context.response.close

Unhandled exception: Closed stream (IO::Error)
  from /Users/jeremywoertink/Development/crystal/lang/src/io.cr:118:5 in 'check_open'
  from /Users/jeremywoertink/Development/crystal/lang/src/http/server/response.cr:163:7 in 'check_headers'
  from /Users/jeremywoertink/Development/crystal/lang/src/http/server/response.cr:77:7 in 'content_length='
  from /Users/jeremywoertink/Development/crystal/lang/src/http/server/response.cr:253:11 in 'close'
  from /Users/jeremywoertink/Development/crystal/lang/src/compress/gzip/writer.cr:109:5 in 'close'
  from /Users/jeremywoertink/Development/crystal/lang/src/http/server/response.cr:126:7 in 'close'
  from zzz.cr:16:1 in '__crystal_main'
  from /Users/jeremywoertink/Development/crystal/lang/src/crystal/main.cr:110:5 in 'main_user_code'
  from /Users/jeremywoertink/Development/crystal/lang/src/crystal/main.cr:96:7 in 'main'
  from /Users/jeremywoertink/Development/crystal/lang/src/crystal/main.cr:119:3 in 'main'

Ref: Lucky CI

[straight-shoota]

The problem is that the internal HTTP::Server::Response::Output stream is wrapped in a different IO. This is wrong.
I don't think this could ever work like that, because it would write the entire HTTP request gzip encoded, including headers and end markers.

HTTP::Server::Response::Output is really not meant to be wrapped in anything, it is closely linked with the respective Response instance and expected to be directly attached as Response#output. I think we should probably restrict or remove the #output= setter to confine that.

The exact flow of control which breaks this goes like this:
response.close calls output.close which in this case is Compress::Gzip::Writer#close. With sync_close it forwards the close call to the wrapped IO (which is HTTP::Server::Response::Output) but only after closing itself. That's the correct order for wrapped streams, to allow for finalization of the wrapper.
But then HTTP::Server::Response::Output thinks it can still write headers and calls Response#content_length= which then in turn tries to write Response#output which is the Compress::Gzip::Writer instance and at this point already closed.

[jwoertink]

I'm not familiar with using GZip, but this is how it's written in Lucky. If we get rid of output= what would be the proper way to ensure the output is compressed here?

[asterite]

Note the according to the docs, and as far as I remember, you could perfectly change the response output. The docs even mention compression as one of the examples.

If this broke in master, I think we should revert whatever broke it. Otherwise we are breaking backwards compatibility.

[straight-shoota]

Sorry, I completely missed that the above code works in 1.2.1. It's a regression from #11253 then.

[straight-shoota]

Sorry for the delay, I lost track of this issue. It should've deserved more priority.

I submitted a patch in #11631. I ensured the spec in lucky/text_response_spec.cr succeeds.

[jwoertink]

Thanks @straight-shoota!