async-resource-vs-destroy benchmark is broken locally

[addaleax]

(Extracted from #33575 (comment))

• Version: master
• Platform: Linux addaleax2 5.3.0-53-generic V8 upgrades and what they mean for versioning #47~18.04.1-Ubuntu SMP Thu May 7 13:10:50 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem: benchmarks

What steps will reproduce the bug?

$ ./node benchmark/async_hooks/async-resource-vs-destroy.js 
Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed
    at write_ (_http_outgoing.js:689:11)
    at ServerResponse.end (_http_outgoing.js:783:5)
    at /home/addaleax/src/node/master/benchmark/async_hooks/async-resource-vs-destroy.js:152:13
    at FSReqCallback.readFileAfterClose [as oncomplete] (internal/fs/read_file_context.js:63:3)
Emitted 'error' event on ServerResponse instance at:
    at emitErrorNt (_http_outgoing.js:666:43)
    at processTicksAndRejections (internal/process/task_queues.js:85:21) {
  code: 'ERR_STREAM_DESTROYED'
}

Bisecting pointed to 28e6626 (#33131).

/cc @ronag

[ronag]

Benchmark is broken in that it does not wait for the requests to finish before closing the server, alternatively does not catch the error caused by ending a closed response.

Looks to me like the benchmark does not wait for all the requests to finish. Hence, once it's dispatched all the requests it closes the server and the response is closed, once the setTimeout fires it will try to end() a closed response and errors.

Is it possible to make close async and have it wait a bit before closing the server? Or just to be quick:

[addaleax]

@ronag Oh, yeah, sorry, should have commented that the suggestion you posted didn’t work, even with large timeouts. :/

I also think closing the server handle itself shouldn’t really affect the individual streams?

[ronag]

I also think closing the server handle itself shouldn’t really affect the individual streams?

Closing the server could/should close the response? The problem here is that the response has been closed. It worked before the commit you refer to because it incorrectly ignored this state.

What platform are you on? I can't reproduce it.

[addaleax]

I also think closing the server handle itself shouldn’t really affect the individual streams?

Closing the server could/should close the response?

… that sounds like a huge bug? But I’m also fairly sure that that’s not what happens.

The problem here is that the response has been closed. It worked before the commit you refer to because it incorrectly ignored this state.

Hm okay, in that case I guess the problem is just being exposed by it?

What platform are you on? I can't reproduce it.

x64 Linux, Ubuntu 18.04

[ronag]

… that sounds like a huge bug? But I’m also fairly sure that that’s not what happens.

Given the error you encounter I suspect it is hitting this 28e6626#diff-feaf3339998a19f0baf3f82414762c22R208-R211 or 28e6626#diff-feaf3339998a19f0baf3f82414762c22R678

Hm okay, in that case I guess the problem is just being exposed by it?

That is my belief.

What platform are you on? I can't reproduce it.

Cool. I'll see if I can reproduce by running it inside docker.

[addaleax]

@ronag onServerResponseClose() and resOnFinish() are per-request/per-socket, not per server, though. The server closing shouldn’t have an effect beyond new connections no longer being accepted.

[ronag]

The server closing shouldn’t have an effect beyond new connections no longer being accepted.

From what I can see the server _handle will be closed (i.e. _handle.close()). What that actually does in practice I don't know and will have to defer to you.

For whatever reason, it seems for you the response and/or socket is prematurely closed which is why you are getting an error.

[addaleax]

@ronag That makes sense, but I think it also means #33131 should have been semver-major.

The reason why the benchmark is failing is that the socket being closed, it because httpAllowHalfOpen isn’t set and thus when the client closes the readable side of the underlying socket, the writable side will be .end()ed as well. I’ve opened #33642 to address that.

Tbh, it feels wrong to me that you can’t write code like await someAsyncThing(); res.end('done');. It’s a pitfall that I probably wouldn’t think of when writing an HTTP server.

(I get that your PR aligned the behavior with the one for streams, but for most Duplex streams, allowHalfOpen is true, so it’s a lot harder to get this wrong there usually.)

Aside, should #33131 have updated .writable as well, in addition to .destroyed? Right now fixing the benchmark means that if (!res.writable) res.end(); won’t work.

[ronag]

@ronag That makes sense, but I think it also means #33131 should have been semver-major.

Agreed. Was not aware of this side effect at the time.

Aside, should #33131 have updated .writable as well, in addition to .destroyed?

I agree writable should also be updated.

[ronag]

#33643

[ronag]

Tbh, it feels wrong to me that you can’t write code like await someAsyncThing(); res.end('done');. It’s a pitfall that I probably wouldn’t think of when writing an HTTP server.

I think this can be fixed in a way which is consistent with the semantics of streams. I'll add this to my list.

[ronag]

Aside, should #33131 have updated .writable as well, in addition to .destroyed?

#33655