Skip to content

async-resource-vs-destroy benchmark is broken locally #33591

Closed
Closed
async-resource-vs-destroy benchmark is broken locally#33591
Labels
benchmarkIssues and PRs related to Node.js benchmarks and benchmarking infrastructure.

Description

@addaleax
addaleax
opened on May 27, 2020

(Extracted from #33575 (comment))

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

Activity

  1. addaleax
    added
    benchmarkIssues and PRs related to Node.js benchmarks and benchmarking infrastructure.
    on May 27, 2020

  2. ronag commented on May 27, 2020

    @ronag
    ronag
    on May 27, 2020
    Member

    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:

  3. addaleax commented on May 27, 2020

    @addaleax
    addaleax
    on May 27, 2020
    MemberAuthor

    @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?

  4. ronag commented on May 27, 2020

    @ronag
    ronag
    on May 27, 2020
    Member

    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.

  5. addaleax commented on May 27, 2020

    @addaleax
    addaleax
    on May 27, 2020
    MemberAuthor

    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

  6. ronag commented on May 27, 2020

    @ronag
    ronag
    on May 27, 2020
    Member

    … 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.

  7. addaleax commented on May 27, 2020

    @addaleax
    addaleax
    on May 27, 2020
    MemberAuthor

    @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.

  8. ronag commented on May 27, 2020

    @ronag
    ronag
    on May 27, 2020
    Member

    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.

  9. addaleax
    added a commit that references this issue on May 29, 2020

    benchmark: fix async-resource benchmark

    73d01d8

  10. addaleax commented on May 29, 2020

    @addaleax
    addaleax
    on May 29, 2020
    MemberAuthor

    @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.

  11. ronag commented on May 29, 2020

    @ronag
    ronag
    on May 29, 2020
    Member

    @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.

  12. ronag commented on May 29, 2020

    @ronag
    ronag
    on May 29, 2020
    Member

    #33643

  13. ronag commented on May 29, 2020

    @ronag
    ronag
    on May 29, 2020
    Member

    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.

  14. ronag commented on May 30, 2020

    @ronag
    ronag
    on May 30, 2020
    Member

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

    #33655

  15. addaleax
    added a commit that references this issue on Jun 6, 2020

    benchmark: fix async-resource benchmark

    f5ed5fe
  16. codebytere
    added a commit that references this issue on Jun 18, 2020

    benchmark: fix async-resource benchmark

    bf56dec
  17. ronag
    added 2 commits that reference this issue on Jun 20, 2020

    http: don't emit error after close

    299b2fd

    http: don't emit error after close

    30cc542
  18. codebytere
    added a commit that references this issue on Jun 30, 2020

    benchmark: fix async-resource benchmark

    527b53c
  19. codebytere
    added 2 commits that reference this issue on Jul 9, 2020

    benchmark: fix async-resource benchmark

    bfffb97

    benchmark: fix async-resource benchmark

    99d34ea
  20. Mesteery
    closed this as completedon Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    benchmarkIssues and PRs related to Node.js benchmarks and benchmarking infrastructure.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions