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

Support Windows Named Pipes for IPC #120

Merged
merged 1 commit into from
Jan 30, 2018
Merged

Conversation

watson
Copy link
Contributor

@watson watson commented Jan 29, 2018

No description provided.

@watson watson force-pushed the windows-pipes branch 2 times, most recently from 9354805 to 656ead2 Compare January 29, 2018 23:12
@watson
Copy link
Contributor Author

watson commented Jan 29, 2018

It seems that a tiny fraction of the requests sent over the Windows pipe fails. Looking at the AppVeyor test output, I see randomly a few requests (usually between 1 and 40 out of 16000) with errors. I'm not sure exactly why, but one explanation could be that named pipes on Windows are simply not as stable as sockets on Unix. Or maybe just that I'm hitting a bug in Node core related to named pipes.

I don't have access to a Windows box my self, so I have a hard time debugging this. If anybody else knows more about named pipes on Windows, input would be highly appreciated. If not I'm not sure there's much I can do 😞

@mcollina
Copy link
Owner

@watson if a tiny fraction of requests becomes errors it is ok. You just need to update the tests to pass if it is a small fraction of the total (let's flag it at 2%, maybe), instead of 0. The errors are typically related to kernel buffers being full, and I have no clue on how to tweak those on Windows.

I see some other test failures, can you please check those as well?

@watson watson force-pushed the windows-pipes branch 5 times, most recently from 399d93b to b9c2177 Compare January 30, 2018 13:28
@mcollina
Copy link
Owner

There is still something fishy on Windows. Only Node 8 and 9 are failing now: https://ci.appveyor.com/project/mcollina/autocannon/build/1.0.237!

@watson
Copy link
Contributor Author

watson commented Jan 30, 2018

@mcollina the other tests that fails are also related to the errors on the pipe in one way or the other.

I've set it the cut-off to be 1% as that's 150 errors, which I haven't seen happen. But if you feel that's too little, I can up it to 2% instead.

For some reason I get a weird "unrelated" error on Node.js 8 and 9 on Windows now. It's the last test in test/run.test.js, which is the last test inside the for-loop in the bottom of the file. Note that this is not related to IPC - These tests have not been modified in the PR.

Here's what I've tried so far:

  • I've tried to reverse the for-loop so it tests in order 5xx, 4xx, 3xx, 2xx, 1xx and it's always the last one that fails
  • I've also tried to skip the last test so that there's only 4 tests inside the for-loop in which case they all pass. So the error only occurs on the 5th test
  • I've also tried to manually call server.close() after each test, to see if it was related to the number of servers being created, but that had no effect

Do you have any idea of what's going on??

@mcollina
Copy link
Owner

absolutely no idea. You can try split those tests into multiple files, in this way the error should be easier to isolate.

@watson
Copy link
Contributor Author

watson commented Jan 30, 2018

@mcollina If I move them to another file, I'm positive the error will go away. But I'm sure that will not be an acceptable solution right?

@mcollina
Copy link
Owner

I'm good with that solution.

@watson
Copy link
Contributor Author

watson commented Jan 30, 2018

@mcollina Found the issue. Tap have some pretty crazy defaults that differ between the CLI and programmatic usage, so that if the test runs for more than 30 seconds if started via the tap executable, it just auto-ends it in a really hard-to-reason-about way.

I've made the tests shorter now and it solved the problem.

@mcollina
Copy link
Owner

Amazing!

@mcollina mcollina merged commit 2fc7333 into mcollina:master Jan 30, 2018
@mcollina
Copy link
Owner

Good work!

@watson watson deleted the windows-pipes branch January 30, 2018 17:43
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