--on-port #145

goto-bus-stop · 2018-07-18T14:31:36Z

Do autocannon --on-port /api/expensive-route -- node server.js to cannonise a script once it's booted up.

This works by delaying the run() call until the script has started listening on a port, and then starting autocannon targeting http://localhost:${THAT_PORT}/api/expensive-route. Once over the script gets a SIGINT like in 0x.

There are a few things I'm not certain about:

0x implements the child process spawning inside its Node API, this patch only implements it in the CLI. This is quite a bit easier to do because all of the run() setup doesn't have to be async this way, but maybe there is a use case for this functionality in the Node API too?
I'm just assuming targeting 'localhost:PORT' is the right thing to do, but maybe it's not? I'm not sure what else it could be, and users can specify a full URL if they need rather than just a path, so it shouldn't be a problem in most cases.
~~Should probably actually use nitm instead~~

Same URL disclaimer as #147:

It uses the URL API which was backported to Node 6.13.0, if lower
versions of Node 6 should be supported I can change it to use
url.parse and url.format instead.

mcollina · 2018-07-19T08:03:16Z

0x implements the child process spawning inside its Node API, this patch only implements it in the CLI. This is quite a bit easier to do because all of the run() setup doesn't have to be async this way, but maybe there is a use case for this functionality in the Node API too?

I think this is only a CLI feature. However, I'm good with placing it in exposing it as a reusable function, similar to what we do for track: https://github.com/mcollina/autocannon/blob/master/autocannon.js#L13. I don't think this should go into run.

I'm just assuming targeting 'localhost:PORT' is the right thing to do, but maybe it's not? I'm not sure what else it could be, and users can specify a full URL if they need rather than just a path, so it shouldn't be a problem in most cases.

That's a safe assumption, yes.

Should probably actually use nitm instead

Go for it!

mcollina · 2018-07-19T08:04:50Z

(CI is failing)

mcollina · 2018-07-19T10:13:32Z

help.txt

@@ -14,6 +14,8 @@ Available options:
        The amount of requests to make before exiting the benchmark. If set, duration is ignored.
  -S/--socketPath
        A path to a Unix Domain Socket or a Windows Named Pipe. A URL is still required in order to send the correct Host header and path.
+  --on-port PATH
+        Start the command listed after -- on the command line. When it starts listening on a port, start sending requests to that port with the given URL path.


We might want to move PATH out from this option. See #146.

yeah when i wrote that line my understanding of how it would work was wrong. it's a boolean flag in the code and PATH is just the URL. will update 👍

goto-bus-stop · 2018-07-20T10:03:51Z

Hmm looking at the CI failures I would guess that latency.average ends up at 0 so the t.ok() check fails. I'll change it to t.type

mcollina · 2018-07-20T10:29:52Z

Can you please update the docs as well?

This reverts commit c930369.

mafintosh · 2018-08-06T18:04:49Z

autocannon.js

@@ -139,9 +151,37 @@ function start (argv) {
    return
  }

+  if (argv.onPort) {
+    const proc = nitm(
+      ['-r', require.resolve('./lib/detectPort')],


You should use NODE_PATH instead, as this will fail on windows with usernames with spaces in them (which lots of people have there).

Move detectPort.js to another folder, fx ./injects and set process.env.NODE_PATH = path.join(__dirname, '/injects') and use ['-r', 'detectPort'] to work around that.

I took it from https://github.com/davidmarkclements/0x/blob/3611fc7f7c04ee741daeda33c972bb26cd7bdd0b/platform/v8.js#L34, so we should also change it there then. Does windows not support quoted arguments? 😱

mafintosh · 2018-08-06T18:07:24Z

lib/detectPort.js

+process.on('exit', () => stream.close())
+stream.once('data', (chunk) => {
+  if (chunk.toString() === 'SIGINT') process.exit(0)
+})


Maybe we should unref the stream here? Just incase the server closes gracefully and we don't wanna keep it open.

Hmm actually not sure how to do this, fs.ReadStream doesn't have an unref(). autocannon will send the SIGINT when it's done anyway so currently the process just keeps running for those additional seconds. Ideally the child process would exit when the server closes and autocannon would also stop, I guess?

mafintosh · 2018-08-06T18:07:51Z

Looks good other than my comments 👍

goto-bus-stop · 2018-08-10T09:42:16Z

Oh, on-net-listen needs async_hooks, so this actually can't support Node 6 at all. Added a check for that too. The other CI failures are fixed by #144

e; Node 8:

events.js:183
      throw er; // Unhandled 'error' event
      ^
Error: shutdown ENOTCONN
    at _errnoException (util.js:992:11)
    at Socket.onSocketFinish (net.js:307:25)
    at emitNone (events.js:106:13)
    at Socket.emit (events.js:208:7)
    at finishMaybe (_stream_writable.js:614:14)
    at afterWrite (_stream_writable.js:465:3)
    at _combinedTickCallback (internal/process/next_tick.js:144:20)
    at process._tickCallback (internal/process/next_tick.js:180:9)

checking

e2; nodejs/node#13542

e3; ok, now the only remaining Travis failure is one fixed by #144 :P waiting for appveyor

This reverts commit 6b2025c.

goto-bus-stop · 2018-08-10T12:23:11Z

@mafintosh oh the published nitm version does not have windows support yet. could you publish a new version? git master seems to work (it maybe loses the extra fd that's used for pseudo SIGINTs, as fs.writeSync(3) will throw in the child process, but need to look into that more closely first)

mafintosh · 2018-08-10T14:05:37Z

@goto-bus-stop oops, sorry about that. just published.

goto-bus-stop · 2018-08-10T14:41:11Z

merci merci!

it seems like on windows there is no guarantee that the additional pipe is opened at fd=3: nodejs/node#14046 (comment)

prolly need to use a temp socket instead

mafintosh · 2018-08-10T18:24:58Z

@goto-bus-stop interesting about the fd stuff. we acutally just use fd 3 in the other modules and it seems to work quite well on windows (maybe just by chance!). Would be good to put these pseudo signals in a module also so we can test it independently etc

goto-bus-stop · 2018-08-10T18:43:06Z

hmm I tried 0x's --on-port and it didn't work either in a Win7 or Win10 VM. it seems like the stdout redir did work there tho, actually, which also relies on fd 3. 0x doesn't use nitm yet, could it be that the extra pipe isn't passed through the proxy cmds that nitm uses on windows?

mafintosh · 2018-08-10T19:01:32Z

@goto-bus-stop really? it's works for me consistently on windows. are you sure you are using the latest version of 0x?

goto-bus-stop · 2018-08-10T19:06:25Z

I'm using git master :/ I'll take a closer look next week at what exactly is going wrong … it sounds like it might be on my end then? tho this is a clean install with just Win10 + Git + Node 8 and using the 0x rest-api example from PowerShell.

goto-bus-stop · 2018-08-15T14:40:47Z

omg, the onPort test works when run as .\node_modules\.bin\tap test\onPort.test.js, but times out when run through npm test… 🤕

e; node.cmd is not started by spawnwrap.cmd when run from an npm script… 🤔
e; running with npm defines several differently cased $PATH environment variables. that may be an issue?
e; yes that's it :)
e; will be fixed in npm 6.4: npm/npm-lifecycle#22

Makes the start() fn a bit easier to understand

goto-bus-stop · 2018-08-17T09:12:09Z

After discussing with @mafintosh i removed nitm in favour of using NODE_OPTIONS, which is supported in all versions with async_hooks. on-net-listen requires async_hooks, so it was already required for --on-port.

Technically it's possible now to drop the socket stuff and just use fd 3 like 0x does, but I'm not entirely sure if fd 3 is really guaranteed or if it just usually coincidentally ends up working, so the socket way may still be more reliable...

I think this is ready, but I merged #144 into this to fix flaky tests on this branch which ruins the diff view, so it would be easiest if #144 was merged to master first and this was PR rebased.

mafintosh · 2018-08-17T16:30:45Z

LGTM, great work @goto-bus-stop

mcollina · 2018-08-20T07:46:39Z

Great work!

goto-bus-stop changed the title ~~--on-port~~ [WIP-ish] --on-port Jul 18, 2018

mcollina reviewed Jul 19, 2018

View reviewed changes

goto-bus-stop mentioned this pull request Jul 20, 2018

Target localhost by default if $PORT is set, closes #146. #147

Merged

goto-bus-stop added 7 commits July 20, 2018 13:33

Add --on-port flag to start once a process is running

5371e0e

Import URL constructor

aa590d2

Add --on-port test

f456348

Use nitm.

936a9a2

Update --on-port doc

17e5c58

Accept hostless URL if --on-port is given.

a05fc8b

Update readme.md with help.txt contents.

50ac96e

goto-bus-stop force-pushed the on-port branch from daf1bbd to 50ac96e Compare July 20, 2018 11:44

goto-bus-stop added 4 commits July 20, 2018 15:19

add debug line for travis bc this is passing locally

c930369

Revert "add debug line for travis bc this is passing locally"

0f309ad

This reverts commit c930369.

Fix "N requests" regex in --on-port test.

efe1537

add another debug line for travis

6b2025c

mafintosh reviewed Aug 6, 2018

View reviewed changes

Fix detectPort injection on windows (probably)

ab1212b

goto-bus-stop changed the title ~~[WIP-ish] --on-port~~ --on-port Aug 10, 2018

Exit if --on-port is used without async_hooks

304826a

goto-bus-stop added 4 commits August 10, 2018 11:51

Revert "add another debug line for travis"

06270f6

This reverts commit 6b2025c.

Fix shutdown ENOTCONN error

1bb1f33

Fix spawning child proc in test on windows

dc0cf5b

use path.join like other tests

54be888

goto-bus-stop added 2 commits August 10, 2018 16:58

Use IPC socket instead of a pipe for windows friends

851ece7

Avoid the half open stuff

9ef4270

goto-bus-stop added 2 commits August 15, 2018 15:10

Update --on-port test to expect new percentiles output

f85b259

Update --on-port test to expect new percentiles output

98a67de

goto-bus-stop added 4 commits August 15, 2018 17:02

injects → lib/preload like 0x

0420249

NODE_OPTIONS is available in 8.0.0

56fb442

Remove nitm SIGINT workaround

a98b679

Move socket setup to a function

515c5f9

Makes the start() fn a bit easier to understand

Pass through existing $NODE_OPTIONS

0027720

mcollina merged commit 57ce004 into mcollina:master Aug 20, 2018

goto-bus-stop mentioned this pull request Aug 22, 2018

List percentiles in output data, fixes #138 #144

Closed

goto-bus-stop deleted the on-port branch August 22, 2018 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--on-port #145

--on-port #145

goto-bus-stop commented Jul 18, 2018 •

edited

mcollina commented Jul 19, 2018

mcollina commented Jul 19, 2018

mcollina Jul 19, 2018

goto-bus-stop Jul 19, 2018

goto-bus-stop commented Jul 20, 2018

mcollina commented Jul 20, 2018

mafintosh Aug 6, 2018

goto-bus-stop Aug 10, 2018

mafintosh Aug 6, 2018

goto-bus-stop Aug 10, 2018

goto-bus-stop Aug 10, 2018

mafintosh commented Aug 6, 2018

goto-bus-stop commented Aug 10, 2018 •

edited

goto-bus-stop commented Aug 10, 2018

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018 via email

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018 via email •

edited

goto-bus-stop commented Aug 15, 2018 •

edited

goto-bus-stop commented Aug 17, 2018 •

edited

mafintosh commented Aug 17, 2018

mcollina commented Aug 20, 2018

--on-port #145

--on-port #145

Conversation

goto-bus-stop commented Jul 18, 2018 • edited

mcollina commented Jul 19, 2018

mcollina commented Jul 19, 2018

mcollina Jul 19, 2018

Choose a reason for hiding this comment

goto-bus-stop Jul 19, 2018

Choose a reason for hiding this comment

goto-bus-stop commented Jul 20, 2018

mcollina commented Jul 20, 2018

mafintosh Aug 6, 2018

Choose a reason for hiding this comment

goto-bus-stop Aug 10, 2018

Choose a reason for hiding this comment

mafintosh Aug 6, 2018

Choose a reason for hiding this comment

goto-bus-stop Aug 10, 2018

Choose a reason for hiding this comment

goto-bus-stop Aug 10, 2018

Choose a reason for hiding this comment

mafintosh commented Aug 6, 2018

goto-bus-stop commented Aug 10, 2018 • edited

goto-bus-stop commented Aug 10, 2018

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018 via email

mafintosh commented Aug 10, 2018

goto-bus-stop commented Aug 10, 2018 via email • edited

goto-bus-stop commented Aug 15, 2018 • edited

goto-bus-stop commented Aug 17, 2018 • edited

mafintosh commented Aug 17, 2018

mcollina commented Aug 20, 2018

goto-bus-stop commented Jul 18, 2018 •

edited

goto-bus-stop commented Aug 10, 2018 •

edited

goto-bus-stop commented Aug 10, 2018 via email •

edited

goto-bus-stop commented Aug 15, 2018 •

edited

goto-bus-stop commented Aug 17, 2018 •

edited