Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add #close to GzipStream and DeflateStream classes #531

Closed
wants to merge 1 commit into from

2 participants

@mcolyer

On Heroku when using unicorn, the deflater middleware will occasionally fail to properly close the body. This results in deadlock errors.

By adding close methods to these classes, we ensure that unicorn calls close in all cases. I'm guessing that the occasional failures are caused by requests that are terminated prematurely and the ensure block within the each method isn't triggered. This fixes that because unicorn wraps the method which calls each with an ensure clause that calls close.

@mcolyer mcolyer Add #close to GzipStream and DeflateStream classes
On Heroku when using unicorn, the deflater middleware will occasionally
fail to properly close the body. This results in deadlock errors.

By adding close methods to these classes, we ensure that unicorn calls
close in all cases. I'm guessing that the occasional failures are caused
by requests that are terminated prematurely and the ensure block within
the each method isn't triggered. This fixes that because unicorn wraps
the method which calls each with an ensure clause that calls close.
1d3a7d4
@raggi
Owner

Why did you change the order of closure?

@mcolyer

I'm not sure that I understand, which lines are you referring to?

@raggi
Owner

You reversed the order of body.close and stream.close in each case.

@mcolyer

My thinking was that I absolutely wanted to be sure that all of the content was flushed before closing the body. As you can see from Line 94, things aren't always happening synchronously and so I figured that it was better to be safe.

Does this have a negative effect somewhere else?

@raggi
Owner

Line 94 occurs because GzipWriter's close calls close on the io object (self in this case).

I apologize for doing it to you, but I have superseded this implementation and will be committing it to master.

@raggi raggi closed this pull request from a commit
@raggi raggi delfater: ensure that parent body is always closed
 * Fixes a bug where body is not enumerated (i.e. HEAD), and as such is never
   closed.
 * Users suffering from the above bug are encouraged to investigate their
   middleware order.
 * Supersedes and closes #531.
7bda8d4
@raggi raggi closed this in 7bda8d4
@mcolyer

I'd rather see it fixed, so no worries about not using the commit. I'll keep an eye out for the next release and ensure that it resolves the issue. Thanks for the help.

@thoughtless thoughtless referenced this pull request from a commit in thoughtless/rack
@raggi raggi delfater: ensure that parent body is always closed
 * Fixes a bug where body is not enumerated (i.e. HEAD), and as such is never
   closed.
 * Users suffering from the above bug are encouraged to investigate their
   middleware order.
 * Supersedes and closes #531.

Conflicts:
	lib/rack/deflater.rb

Cherry-picked from 7bda8d4
a882c0d
@thoughtless thoughtless referenced this pull request from a commit in thoughtless/rack
@raggi raggi delfater: ensure that parent body is always closed
 * Fixes a bug where body is not enumerated (i.e. HEAD), and as such is never
   closed.
 * Users suffering from the above bug are encouraged to investigate their
   middleware order.
 * Supersedes and closes #531.

Conflicts:
	lib/rack/deflater.rb

Cherry-picked from 7bda8d4
038c1b8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 21, 2013
  1. @mcolyer

    Add #close to GzipStream and DeflateStream classes

    mcolyer authored
    On Heroku when using unicorn, the deflater middleware will occasionally
    fail to properly close the body. This results in deadlock errors.
    
    By adding close methods to these classes, we ensure that unicorn calls
    close in all cases. I'm guessing that the occasional failures are caused
    by requests that are terminated prematurely and the ensure block within
    the each method isn't triggered. This fixes that because unicorn wraps
    the method which calls each with an ensure clause that calls close.
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 10 deletions.
  1. +23 −10 lib/rack/deflater.rb
View
33 lib/rack/deflater.rb
@@ -72,21 +72,30 @@ def initialize(body, mtime)
def each(&block)
@writer = block
- gzip =::Zlib::GzipWriter.new(self)
- gzip.mtime = @mtime
+ @gzip =::Zlib::GzipWriter.new(self)
+ @gzip.mtime = @mtime
@body.each { |part|
- gzip.write(part)
- gzip.flush
+ @gzip.write(part)
+ @gzip.flush
}
ensure
- @body.close if @body.respond_to?(:close)
- gzip.close
+ close
@writer = nil
end
def write(data)
@writer.call(data)
end
+
+ def close
+ begin
+ @gzip.close if @gzip && !@gzip.closed?
+ rescue Zlib::GzipFile::Error
+ # For some reason this error is still thrown despite the .closed?
+ # check on the previous line.
+ end
+ @body.close if @body.respond_to?(:close)
+ end
end
class DeflateStream
@@ -103,13 +112,17 @@ def initialize(body)
end
def each
- deflater = ::Zlib::Deflate.new(*DEFLATE_ARGS)
- @body.each { |part| yield deflater.deflate(part, Zlib::SYNC_FLUSH) }
- yield deflater.finish
+ @deflater = ::Zlib::Deflate.new(*DEFLATE_ARGS)
+ @body.each { |part| yield @deflater.deflate(part, Zlib::SYNC_FLUSH) }
+ yield @deflater.finish
nil
ensure
+ close
+ end
+
+ def close
+ @deflater.close if @deflater && !@deflater.closed?
@body.close if @body.respond_to?(:close)
- deflater.close
end
end
end
Something went wrong with that request. Please try again.