async-resource-vs-destroy benchmark is broken locally #33591
Comments
|
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.
|
|
@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? |
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. |
… that sounds like a huge bug? But I’m also fairly sure that that’s not what happens.
Hm okay, in that case I guess the problem is just being exposed by it?
x64 Linux, Ubuntu 18.04 |
Given the error you encounter I suspect it is hitting this 28e6626#diff-feaf3339998a19f0baf3f82414762c22R208-R211 or 28e6626#diff-feaf3339998a19f0baf3f82414762c22R678
That is my belief.
Cool. I'll see if I can reproduce by running it inside docker. |
|
@ronag |
From what I can see the server For whatever reason, it seems for you the response and/or socket is prematurely closed which is why you are getting an error. |
In the benchmark, because it performs asynchronous operations before writing its HTTP replies, the underlying socket can be closed by the peer before the response is written. Since 28e6626, that means that attempting to `.end()` the HTTP response results in an uncaught exception, breaking the benchmark. Fix that by checking whether the response object has been destroyed or not before attempting to call `.end()`. nodejs#33591
|
@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 Tbh, it feels wrong to me that you can’t write code like (I get that your PR aligned the behavior with the one for streams, but for most Duplex streams, Aside, should #33131 have updated |
I think this can be fixed in a way which is consistent with the semantics of streams. I'll add this to my list. |
In the benchmark, because it performs asynchronous operations before writing its HTTP replies, the underlying socket can be closed by the peer before the response is written. Since 28e6626, that means that attempting to `.end()` the HTTP response results in an uncaught exception, breaking the benchmark. Fix that by checking whether the response object has been destroyed or not before attempting to call `.end()`. #33591 PR-URL: #33642 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
In the benchmark, because it performs asynchronous operations before writing its HTTP replies, the underlying socket can be closed by the peer before the response is written. Since 28e6626, that means that attempting to `.end()` the HTTP response results in an uncaught exception, breaking the benchmark. Fix that by checking whether the response object has been destroyed or not before attempting to call `.end()`. #33591 PR-URL: #33642 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
Refs: #33591 PR-URL: #33654 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
In the benchmark, because it performs asynchronous operations before writing its HTTP replies, the underlying socket can be closed by the peer before the response is written. Since 28e6626, that means that attempting to `.end()` the HTTP response results in an uncaught exception, breaking the benchmark. Fix that by checking whether the response object has been destroyed or not before attempting to call `.end()`. #33591 PR-URL: #33642 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Juan José Arboleda <[email protected]>
(Extracted from #33575 (comment))
What steps will reproduce the bug?
Bisecting pointed to 28e6626 (#33131).
/cc @ronag
The text was updated successfully, but these errors were encountered: