rack-devel archive mirror (unofficial) https://groups.google.com/group/rack-devel
 help / color / mirror / Atom feed
From: Wojtek Kruszewski <wojtek@oxos.pl>
To: rack-devel@googlegroups.com
Subject: Re: Not cleaning up tempfiles for multipart?
Date: Mon, 6 Jan 2014 14:45:41 -0800 (PST)	[thread overview]
Message-ID: <73e2d44a-2842-4348-9133-9454be615645@googlegroups.com> (raw)
In-Reply-To: <20100318095409.GB15049@dcvr.yhbt.net>

[-- Attachment #1: Type: text/plain, Size: 4352 bytes --]

I see this is still an issue. Is there a recommended way to have multipart 
temp files cleaned after request?

On Thursday, March 18, 2010 10:54:09 AM UTC+1, Eric Wong wrote:
>
> Charles Oliver Nutter <hea...@headius.com <javascript:>> wrote:
> > On Sun, Mar 7, 2010 at 7:12 PM, Eric Wong <normal...@yhbt.net<javascript:>> 
> wrote:
> > > I guess this deals with wonky multipart uploads that browsers still
> > > generate these days[1]. �Ugh, yeah, it's nasty...
> > > The comment below in _call is very important.
> > >
> > >
> > > 1. �Ensure all tempfiles created by Rack go into an array in env,
> > > � �probably env["rack.tempfiles"]:
> > >
> > > � �tempfile = Tempfile.new("foo")
> > > � �(env["rack.tempfiles"] ||= []) << tempfile
> > 
> > I'm with you so far...
> > 
> > > 2. �Have a middleware wrap everything, including the response body:
> > 
> > I'm a bit new to the middleware thing. Does this mean everyone would
> > have to configure middleware on their setups to get this
> > tempfile-closing behavior? And your comment below...does that mean
> > there are situations where this wouldn't work?
> > 
> > Closing and removing tempfiles when they're no longer needed should be
> > the default behavior, not something you have to configure. It's a bug
> > to not close and remove them (or at least, a bug to keep creating new
> > ones and expecting GC to clean everything up eventually).
>
> I would expect major frameworks to stick this into the default
> middleware stack as a convenience, but some users want a more bare-bones
> Rack stack can still opt out.  I see it as needless overhead as the vast
> majority of HTTP requests do not create tempfiles.
>
> I don't make decisions for Rack, though.
>
> <snip>
>
> > > � � �# the Rack server should call this (when we're the body)
> > > � � �def close
> > > � � � �tempfiles = env["rack.tempfiles"]
> > > � � � �if tempfiles
> > > � � � � �tempfiles.each { |tmp| tmp.close! rescue nil }
> > > � � � �end
> > > � � �end
> > 
> > By "the Rack server should call this" do you mean that if the server
> > doesn't call this, tempfiles Rack creates will not be cleaned up until
> > GC runs?
>
> Yes, a Rack-compliant server should call body.close if it responds to
> body.close at the end of the response cycle for every individual
> request.
>
> I put this in the wrapped body because body.each {} could be relying on
> the tempfiles to generate the response.  body.close is the absolute last
> action for any Rack application.
>
> > Shouldn't Rack itself be guaranteeing it doesn't leave garbage files 
> around?
> > 
> > > � � �# wrap the normal application call, saving env
> > > � � �def _call(env)
> > > � � � �self.env = env
> > >
> > > � � � �# XXX VERY IMPORTANT:
> > > � � � �# you need to ensure env stays the same throughout the 
> request,
> > > � � � �# some middlewares overwrite/replace it instead of 
> merge!-ing into it
> > > � � � �status, headers, body = app.call(env)
> > > � � � �self.body = body
> > > � � � �[ status, headers, self ]
> > > � � �end
> > > � �end
> > 
> > Same question as above...can badly-written middleware now cause
> > tempfiles to linger?
>
> Yes, it might be safer to do this before app.call above:
>
> �  env["rack.tempfiles"] ||= []
>
> That way if env gets replaced down the stack, e.g. via:
>
>    app.call(env.merge("foo.hello" => "world"))
>    # env.merge! would be correct above
>
> any use of env["rack.tempfiles"] will still point to the same array.
> Well, almost...
>
> Then again some bad middleware could do:
> �  env["rack.tempfiles"] += [ tmp_a, tmp_b ]
>
> Instead of what they _should_ do:
> �  env["rack.tempfiles"].concat([ tmp_a, tmp_b ])
>
> So yes, discourage future middleware authors from replacing
> the rack.tempfiles array.
>
> -- 
> Eric Wong
>
>

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[-- Attachment #2: Type: text/html, Size: 5361 bytes --]

  reply	other threads:[~2014-01-07  1:30 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-03-05 14:40 Not cleaning up tempfiles for multipart? Charles Oliver Nutter
2010-03-05 14:47 ` Charles Oliver Nutter
2010-03-05 14:48   ` Charles Oliver Nutter
2010-03-06 10:20   ` Hongli Lai
2010-03-07 14:25     ` Charles Oliver Nutter
2010-03-06  7:55 ` Eric Wong
2010-03-06 10:25   ` Hongli Lai
2010-03-07 14:34     ` Charles Oliver Nutter
2010-03-08  0:22       ` Eric Wong
2010-03-08  1:12         ` Eric Wong
2010-03-17 15:41           ` Charles Oliver Nutter
2010-03-18  9:54             ` Eric Wong
2014-01-06 22:45               ` Wojtek Kruszewski [this message]
2014-02-11 21:05                 ` Eric Wong
2014-03-27 21:40                   ` Lenny Marks
2010-03-07 23:53     ` Eric Wong
2010-03-08 11:26       ` Hongli Lai
2010-03-08 11:30         ` Hongli Lai
2010-03-08 14:33           ` Randy Fischer
2010-03-08 14:43           ` Charles Oliver Nutter
2010-03-08 14:49             ` James Tucker
2010-03-17  2:37           ` Eric Wong
2010-03-08 13:22         ` Charles Oliver Nutter
2010-03-08 14:42         ` James Tucker
2010-03-08 17:24           ` Hongli Lai
2010-03-09  7:43             ` Charles Oliver Nutter
2010-03-08 10:05     ` James Tucker
2010-03-07 14:27   ` Charles Oliver Nutter
2010-03-08  0:18     ` Eric Wong
2010-03-08 10:07       ` James Tucker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://groups.google.com/group/rack-devel

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=73e2d44a-2842-4348-9133-9454be615645@googlegroups.com \
    --to=rack-devel@googlegroups.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).