Repository navigation
Add response_finished support to TempfileReaper - #2363
Conversation
88e791f to
13fd68a
Compare
ioquatix
left a comment
ioquatix
left a comment
There was a problem hiding this comment.
LGTM, @jeremyevans can you merge if you also agree.
jeremyevans
left a comment
jeremyevans
left a comment
There was a problem hiding this comment.
I think it's good to add support for this. I have some suggestions for minor improvements, but they are optional, no objections to merging as-is.
There was a problem hiding this comment.
If the constant is private, this is only called when env[RACK_TEMPFILES] is set, so I don't think we need the &. here, unless you are trying to handle a case where the key is deleted by the app or other middleware:
| env[RACK_TEMPFILES]&.each(&:close!) | |
| env[RACK_TEMPFILES].each(&:close!) |
There was a problem hiding this comment.
I did it this way because there are existing tests for the BodyProxy implementation that TempfileReaper should be resilient to other middleware messing with the array
rack/test/spec_tempfile_reaper.rb
Line 88 in fac9a7a
Since this implementation is new, I suppose we could deviate, but I don't have a strong opinion either way
| env[RACK_TEMPFILES] ||= [] | ||
|
|
||
| begin | ||
| _, _, body = response = @app.call(env) | ||
| rescue Exception | ||
| env[RACK_TEMPFILES]&.each(&:close!) | ||
| raise | ||
| end | ||
| if response_finished = env[RACK_RESPONSE_FINISHED] | ||
| response_finished << RESPONSE_FINISHED_HANDLER |
There was a problem hiding this comment.
| env[RACK_TEMPFILES] ||= [] | |
| begin | |
| _, _, body = response = @app.call(env) | |
| rescue Exception | |
| env[RACK_TEMPFILES]&.each(&:close!) | |
| raise | |
| end | |
| if response_finished = env[RACK_RESPONSE_FINISHED] | |
| response_finished << RESPONSE_FINISHED_HANDLER | |
| tempfiles = env[RACK_TEMPFILES] ||= [] | |
| if response_finished = env[RACK_RESPONSE_FINISHED] | |
| response_finished << proc do | |
| tempfiles.each(&:close!) | |
| end |
I'm not in favour of my proposed approach, but I thought I'd mention it. Also the diff is messed up in this comment but it's the idea that I'm trying to share.
|
I was also wondering if for whatever reason, we had multiple For example would close be better off like this: Otherwise, the array is never cleared. Does it matter? |
When supported by the server, this delays closing any tempfiles until after the server socket has been closed (assuming it will be), and additionally removes an allocation (the BodyProxy).
13fd68a to
01c14b8
Compare
When supported by the server, this delays closing any tempfiles until after the server socket has been closed (assuming it will be), and additionally removes an allocation (the BodyProxy).
(hiding whitespace from the diff makes the changes more clear)