Will request.pause() get fixed?

Showing 1-12 of 12 messages
Will request.pause() get fixed? Denis Washington 2/6/12 9:11 AM
Hi,

I am trying to implement authenticated file upload, which means that I
want to check any request before reading in the POST body. As the
checking requires IO, though, I cannot (and don't want to) listen to the
request's 'data' event before the 'request' handler terminates.

Now I thought that request.pause() is just what I need, but sadly I
found out that it still leaves through some 'data' events. Here is the
thread from early 2011 that I found regarding that issue:

http://groups.google.com/group/nodejs/browse_thread/thread/cafe8397e3bec189

This doesn't seem to be fixed as of Node 0.6.10, so my question is:
*will* it be fixed? Or do I need to hack around that issue by buffering
the data that pause() lets through? I would rather not, and as I am
probably not the only one who would have to code up that exact
workaround, I think it is fair enough to say that this should be in Node
core.

Regards,
Denis Washington

Re: Will request.pause() get fixed? Igor Zinkovsky 2/6/12 11:01 AM
I believe this was fixed in master (https://github.com/joyent/node/
commit/e6b6075024e9f1330575b10d7e6552e1ea6dad56
), but will not be back-
ported to v0.6.  Latest unstable build (v0.7.2) should contain the fix.
Re: [nodejs] Re: Will request.pause() get fixed? Denis Washington 2/6/12 1:18 PM

It works in 0.7.2, thanks! I didn't see any mention in the release notes
on the Node blog.

Regards,
Denis

Re: Will request.pause() get fixed? tjholowaychuk 2/6/12 2:55 PM
oh yay! finally! haha
Re: [nodejs] Re: Will request.pause() get fixed? Mikeal Rogers 2/6/12 6:05 PM
This really needs to be fixed in Stream.pipe() as well.

The fact that we insure this for http.ServerRequest and nowhere else is not good.

We also need to get pause() to take a size argument for how large it should allow the buffer to get before it causes the remote end to stop sending data. This has all been discussed and more or less approved in pull requests that weren't finished.

It would be great if, for 0.8, we could get Stream.pause(size) implemented/fixed and make it consistent across all of node.

-Mikeal

> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en

Re: Will request.pause() get fixed? Mophy Xiong 2/6/12 8:55 PM
We really think such bug fixes should be ported to CURRENT STABLE
build, before the newly stable version is out. Because people are not
recommended to work on unstable builds for productions, but current
stable wouldn't get fixed. Then all we can do is wait? :-) Well, if
the porting is tough due to differences between 0.7 and 0.6, then
forget this ;-)


Regards,
Mophy
Re: [nodejs] Will request.pause() get fixed? Mikeal Rogers 2/6/12 10:47 PM
This is not a bugfix, IMO, it's a feature. A feature we all want, a feature that fixes something newcomers trip on often, but previous released *intended* for the behavior to be the way it was and we have now changed it because this new way is better.

It's a feature, it won't be backported.

> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en

Re: Will request.pause() get fixed? Mophy Xiong 2/6/12 11:08 PM
Well, if this is not treated as a bugfix, it makes sense not
backporting it.

Regards,
Mophy
Re: Will request.pause() get fixed? Bert Belder 2/7/12 3:09 AM
On Feb 7, 7:47 am, Mikeal Rogers <mikeal.rog...@gmail.com> wrote:
> This is not a bugfix, IMO, it's a feature. A feature we all want, a feature that fixes something newcomers trip on often, but previous released *intended* for the behavior to be the way it was and we have now changed it because this new way is better.
>
> It's a feature, it won't be backported.

I would like to *guarantee* that pause() pauses immediately in node
0.8. Most of the time it pauses immediately anyway, and it's a huge
PITA dealing with the remaining edge cases where a data event might
happen after a pause() call. If people really like the old situation
we could make the old behavior optional, e.g.
`stream.pause(allowData=false)`.
Re: [nodejs] Re: Will request.pause() get fixed? Isaac Schlueter 2/7/12 8:20 AM
On Tue, Feb 7, 2012 at 03:09, Bert Belder <bertb...@gmail.com> wrote:
> If people really like the old situation
> we could make the old behavior optional, e.g.
> `stream.pause(allowData=false)`.

No one wants the old behavior.  It's only confusing.

We do need to put some kind of limit on how much will be buffered,
however.  It's too easy to pause a request, then fail to resume it in
a reasonable time, and have it take up all the memory.  This won't
happen very often when dealing with TCP streams, since they'll push
back on the wire, but if we make this a general guarantee on userland
streams, then it needs to be implemented in Stream.stream, and take an
argument for how many bytes to buffer.

stream.pause(maxBuffer=1024) or something.  Then keep an array of
chunks (like Koichi did for http request streams), and if the total
length goes over that number, then start emitting data events.  This
will impact performance a bit when it happens, but since it's a
relatively rare situation, I think that's probably fine.

Re: [nodejs] Re: Will request.pause() get fixed? Mikeal Rogers 2/7/12 10:46 AM
What you mean by "pause" is not what node meant as "pause" in the past. In the past, pause meant "send a signal to the input stream to stop sending me data", that did not insure that data that was enroute wasn't still going to be emitted.

From a high level, your vision of "pause" makes a lot more sense, especially to people that aren't hardcore network geeks like half of the people who work on core :), and that's why there is no resistance whatsoever to fixing core to be pause(maxSize) BUT this is a semantic change and is considered a *feature* and will not be backported, nor would we accept the change a bugfix release because there may be some hidden potential for bugs that assume the old behavior.

-Mikeal

> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en

Re: [nodejs] Will request.pause() get fixed? willwen 2/7/12 9:36 PM
The bug  has been fixed on v0.7.0.
--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nodejs@googlegroups.com

To unsubscribe from this group, send email to
nodejs+unsubscribe@googlegroups.com

For more options, visit this group at
http://groups.google.com/group/nodejs?hl=en?hl=en