Will request.pause() get fixed?
Denis Washington
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
Igor Zinkovsky
commit/e6b6075024e9f1330575b10d7e6552e1ea6dad56), but will not be back-
ported to v0.6. Latest unstable build (v0.7.2) should contain the fix.
Denis Washington
It works in 0.7.2, thanks! I didn't see any mention in the release notes
on the Node blog.
Regards,
Denis
tjholowaychuk
Mikeal Rogers
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
Mophy Xiong
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
Mikeal Rogers
It's a feature, it won't be backported.
Mophy Xiong
backporting it.
Regards,
Mophy
Bert Belder
> 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.
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)`.
Isaac Schlueter
> 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.
Mikeal Rogers
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
