Skip to content
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

test-child-process-fork-getconnections intermittently fails #1100

Closed
Fishrock123 opened this issue Mar 8, 2015 · 14 comments
Closed

test-child-process-fork-getconnections intermittently fails #1100

Fishrock123 opened this issue Mar 8, 2015 · 14 comments
Labels
test Issues and PRs related to the tests.

Comments

@Fishrock123
Copy link
Contributor

Sometimes getting this when testing master (fe36076) on OS X 10.10.2

Jeremiahs-MacBook-Pro:io.js Jeremiah$ make test
/Applications/Xcode.app/Contents/Developer/usr/bin/make -C out BUILDTYPE=Release V=1
make[1]: Nothing to be done for `all'.
ln -fs out/Release/iojs iojs
/usr/bin/python tools/test.py --mode=release message parallel sequential -J
=== release test-child-process-fork-getconnections ===                         
Path: sequential/test-child-process-fork-getconnections
/Users/Jeremiah/Documents/io.js/test/sequential/test-child-process-fork-getconnections.js:18
          throw new Error('[c] closing by accident!');
                ^
Error: [c] closing by accident!
    at Socket.<anonymous> (/Users/Jeremiah/Documents/io.js/test/sequential/test-child-process-fork-getconnections.js:18:17)
    at emitNone (events.js:72:20)
    at Socket.emit (events.js:163:7)
    at _stream_readable.js:891:16
    at process._tickCallback (node.js:349:13)
/Users/Jeremiah/Documents/io.js/test/sequential/test-child-process-fork-getconnections.js:36
      throw new Error('child died unexpectedly!');
            ^
Error: child died unexpectedly!
    at ChildProcess.<anonymous> (/Users/Jeremiah/Documents/io.js/test/sequential/test-child-process-fork-getconnections.js:36:13)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:169:7)
    at Process.ChildProcess._handle.onexit (child_process.js:1044:12)
Command: out/Release/iojs /Users/Jeremiah/Documents/io.js/test/sequential/test-child-process-fork-getconnections.js

cc @bnoordhuis?

@Fishrock123 Fishrock123 added the test Issues and PRs related to the tests. label Mar 8, 2015
@Fishrock123
Copy link
Contributor Author

Fwiw, this has happened on the CI a bunch too.

@Fishrock123
Copy link
Contributor Author

See also node's tracking issue: nodejs/node-v0.x-archive#16805

@Fishrock123
Copy link
Contributor Author

Not sure if this helps at all, but I applied this little debugging patch:

diff --git a/test/sequential/test-child-process-fork-getconnections.js b/test/sequential/test-child-process-fork-getconnections.js
index a587713..0f10fce 100644
--- a/test/sequential/test-child-process-fork-getconnections.js
+++ b/test/sequential/test-child-process-fork-getconnections.js
@@ -34,7 +34,7 @@ if (process.argv[2] === 'child') {

   child.on('exit', function(code, signal) {
     if (!childKilled)
-      throw new Error('child died unexpectedly!');
+      throw new Error(`child died unexpectedly with code: ${code} and signal: ${signal}!`);
   });

   var server = net.createServer();

And it exits with code 1 and a null signal.

@thefourtheye
Copy link
Contributor

I glanced through the source code, but I couldn't find closingOnPurpose defined anywhere. Also, normally, this in an Event's trigger would be undefined or global object, right?

@silverwind silverwind added the macos Issues and PRs related to the macOS platform / OSX. label Jul 13, 2015
@brendanashworth
Copy link
Contributor

@thefourtheye yeah, when I went through the test before, closingOnPurpose is always undefined (not sure why its there). It errors whenever .end() is emitted.

I think this in an event listener refers to the EventEmitter instance, but since you inherit from it, it refers to whatever object you're listening on.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 16, 2015

I'm not certain, but I think there is a race condition somewhere in the code that handles passing sockets between processes (in core, not the test). I've basically rewritten this test from scratch and am seeing sockets being closed just by sending them to the child process.

@Fishrock123 Fishrock123 changed the title test-child-process-fork-getconnections intermittently fails on OS X test-child-process-fork-getconnections intermittently fails Jul 16, 2015
@Fishrock123 Fishrock123 removed the macos Issues and PRs related to the macOS platform / OSX. label Jul 16, 2015
@brendanashworth brendanashworth added macos Issues and PRs related to the macOS platform / OSX. and removed macos Issues and PRs related to the macOS platform / OSX. labels Jul 20, 2015
@Trott
Copy link
Member

Trott commented Aug 27, 2015

The closingOnPurpose is really puzzling. It was added in commit b319264. In that commit, this file is the only place in the entire code base where the string closingOnPurpose appears. And on current master, that is still the case. So, yeah, if that even fires, the test bombs, no matter what. Which seems to be what's happening here.

I realize it was more than two years ago when that commit happened so any recollection may be very foggy, and he's busy CEO-ing these days, but gonna slip a /cc @isaacs in the off chance he'd like to take a look and maybe shed some light. Leftover debugging code? Always failing is actually correct behavior? Something else?

@Fishrock123
Copy link
Contributor Author

closingOnPurpose

I can't even find it by searching the codebase at or around the original commit's time. I suspect it is leftover debug code. Let's remove it.

@Trott
Copy link
Member

Trott commented Aug 27, 2015

But if we yank it, does that mean the event should always throw and error or it should just be removed because it should never throw an error?

I'm inclined to try to find the right place in the code to set that property, actually. Like, "if we're here, we are trying to close this socket, so no error". But maybe there isn't one and you're right. So... ¯\_(ツ)_/¯

@brendanashworth
Copy link
Contributor

Yeah, let's ditch it, it should always throw. I wouldn't set a property in the code because it appeared in a test without any comment / commit log (grunt).

@Trott
Copy link
Member

Trott commented Aug 29, 2015

#2609 should fix this test. I still need to submit an issue for the bug itself, but this test can be written and still test all the same stuff without tripping up on this bug.

@Trott
Copy link
Member

Trott commented Aug 29, 2015

#2610 opened with slightly more minimal test code to trip the bug that caused the issue here.

@Trott
Copy link
Member

Trott commented Sep 2, 2015

Anyone feel good enough about #2609 to give it a LGTM so we can close this issue?

I know with all the shenanigans going on right now with 4.0 looming and a new PR-landing process, this is probably the worst possible time to ask, but that won't stop me.

Trott added a commit to Trott/io.js that referenced this issue Sep 3, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

nodejs#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: nodejs#2609
Fixes: nodejs#1100
Trott added a commit that referenced this issue Sep 3, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
@Trott
Copy link
Member

Trott commented Sep 3, 2015

Fixed by 8ca9ea2

@Trott Trott closed this as completed Sep 3, 2015
Trott added a commit that referenced this issue Sep 3, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Fishrock123 pushed a commit to Fishrock123/node that referenced this issue Sep 3, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

nodejs#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: nodejs#2609
Fixes: nodejs#1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Trott added a commit that referenced this issue Sep 6, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Trott added a commit that referenced this issue Sep 6, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Trott added a commit that referenced this issue Sep 6, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Trott added a commit that referenced this issue Sep 6, 2015
This retains the key elements of test-child-process-fork-getconnections
(forks a child process, sends a bunch of sockets, uses getConnections()
to enumerate them) but contains some code to work around an apparent
intermittent bug that occurs on OS X where a socket seems to close
itself unexpectedly.

#2610 was opened for the bug that
was causing the problem in the first place.

PR-URL: #2609
Fixes: #1100
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Brendan Ashworth <brendan.ashworth@me.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants