Skip to content

Conversation

@mappum
Copy link
Member

@mappum mappum commented Mar 25, 2016

The tests close correctly on Linux now, since we ensure Electron closes by sending it a SIGKILL signal. I'm still not sure what's keeping it open, but it might make sense to just use SIGKILL by default in electron-eval. (Would you rather I make that change and just update the dependency here?)

@mappum
Copy link
Member Author

mappum commented Mar 26, 2016

Not sure why this failed, is the ECONNRESET error actually caused by this PR? (Because it passes for me on multiple machines).

@feross
Copy link
Member

feross commented Mar 26, 2016

@mappum I think defaulting to SIGKILL in electron-eval makes sense, if that's what's required to make it work seamlessly on Linux. I can't think of any downsides to doing that.

Will re-run the tests to see if ECONNRESET was a false positive.

@feross
Copy link
Member

feross commented Mar 26, 2016

I think the SIGKILL is causing the ipc to error somehow, since it's an unclean exit. :(

I'll try hacking around this for now by adding:

wrtc.electronDaemon.on('error', function () {})

But maybe this is an electron / electron-spawn bug that we should file? Or maybe you can just add the above code to suppress errors after the user calls wrtc.close()?

@feross feross merged commit ba9b5b0 into webtorrent:master Mar 26, 2016
@feross
Copy link
Member

feross commented Mar 27, 2016

Sorry, I had to revert this PR. The ECONNRESET error is indeed cause by this PR. The PR works for me, but commenting out the close('SIGKILL') line makes the tests pass.

Something strange is going on here.

@mappum
Copy link
Member Author

mappum commented Mar 27, 2016

Yeah, could definitely be electron. Closing the process seems to be connected to the ECONNRESET, I think this is a similar problem to this issue: mappum/electron-webrtc#15. That one isn't SIGKILLing, but the process is closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants