-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
'drain' event is not being emitted, hanging process #4
Comments
+1
and it worked! |
I'd love for someone who knows more about Node.js streams than me to take a look at this as well. /cc @shama @vladikoff @tbranyen you guys have any opinions on this? |
I have a vague problem with hanging processes on Windows that I can solve with the same change to
I can 100% reproduce mine using local If I take the hanging version, go in the dependencies and change the Original ticket: gruntjs/grunt-contrib-connect#59 (with isolated demo from earlier) |
After upgrading to grunt 0.4.2 (with node-exit), time-grunt (https://github.com/sindresorhus/time-grunt) stops printing elapsed times. It seems like the function to print the statistics (registered on process 'exit') is not called anymore. Also I get hanging node processes on Windows when calling grunt via the exec-maven plugin (http://mojo.codehaus.org/exec-maven-plugin/). |
@alimfeld can you check and see if the |
I'd love to fix this issue, but I need a PR for it that has been tested in Windows 7 and Windows 8, in cmd.exe, PowerShell and git-bash. I don't have access to all these combinations here, so I'm going to need some help. Can anyone help with this? |
@Bartvds so you mentioned a fix worked for you, could you test out |
@vladikoff Sure, but it will take a moment to get a setup. I will get back with this asap. |
@Bartvds thanks so much! |
@vladikoff Things are getting weirder by the minute:
Neither does it when changing the line in My test code: https://github.com/Bartvds/demo-time-grunt-broken This already breaks before I try redirecting. Also I tried my earlier issue with I'm no expert in CLI tricks like redirects, so what I did was just this (please confirm this is correct):
It will run with no CLI output, exit and in Side info: from way back before
I mention this as maybe it sheds some light on what is going on (it is beyond me) |
In my last message I forgot to note this confirms @alimfeld 's report. |
It seems to be the following line in node-exit that causes time-grunt to not output the statistics anymore:
I guess this breaks all functions that try to write something in a process 'exit' callback (e.g. time-grunt). @cowboy I will check tomorrow (when I have access to my Windows machine), whether the |
@vladikoff I have a demo set for Used on standard console it exits as expected on for grunt But hangs on both when redirecting I tried the possible fix but is was no good. 😢 (time for the customary referral to the origin of all this fun: nodejs/node-v0.x-archive#3584) 😞 |
@Bartvds are you saying that the grunt-karma bug you're testing wasn't introduced in Grunt 0.4.2? |
@cowboy I can only say it seems like that is the case. But I would like some confirmation on this before you build on it: this was not my original bug: maybe @stephen-styrchak-hbo who reported the (if it works for him I'm curious what the differences with my minimal demo would be) |
@cowboy I stripped down my demo even further, now it actually does exit in grunt |
Works in PowerShell |
I've added a pull request for this issue that writes an empty string to the stream and uses the callback to call |
Hey @chrisgladd , testing it out. Will let you know how it goes |
@chrisgladd I've tested this for my cases (Windows Vista 64):
The fix solved my original The last Only thing remaining broken is |
The fix also works for me in cmd and powershell. @Bartvds I noticed time-grunt uses // cc @cowboy @sindresorhus |
I think we also know that using |
@vladikoff how do you suggest I do that? I'm just using the Node process exit event:
|
Trying to summarize I think we have three issues we're discussing here:
Back on my Windows (7) machine I tried the following (in PowerShell, cmd and git bash):
However, as stated above I'm not sure, whether preventing further writes is actually a bug or a feature... |
@alimfeld does output redirection work for you with your changes? |
@vladikoff Output redirection works. However, I don't get the output written by time-grunt in the process exit callback. So I basically get all that's written up until the call to exit (i.e. all output from Grunt and Grunt tasks). |
Results with @alimfeld patch: File Contents:
File Contents:
|
Same results for So PowerShell with redirection doesn't get |
There are 2 problems here.
Let's use this issue to fix number 1, and fix number 2 in issue #7. |
Closed in 8b9b539. |
v0.1.2 in npm, please test ASAP! |
@cowboy I confirm node-exit v0.1.2 solved my main issues:
Thanks for the progress so far! Epic bughunt! |
Tested locally and with TeamCity issue, looks good. Thanks! |
My TeamCity issue has gone, everything works like a charm. Thanks, guys! |
The only place I've run into this issue is in a grunt (0.4.2) task running on my TeamCity server (a windows machine) so it may have something to do with the process.on('exit') fix not being called.
From what I can gather about the 'drain' event, I don't believe it's fired unless the stream has broken the stream._writableState.highWaterMark. So if the stream.bufferLength is not zero, but the highWaterMark hasn't been crossed, the 'drain' event will not be emitted and the process will never exit.
if (stream.bufferSize === 0) {
with
if (!stream._writableState.needDrain) {
Seems to solve the issue for me and still preserve the task output but I'm not sure that's 100% correct either.
The text was updated successfully, but these errors were encountered: