http2: do not emit 'aborted' if the stream closed locally

[lovinglyy]

check if the stream didn't close locally before calling aborted

Fixes: #22851

Checklist

• commit message follows commit guidelines

[Trott]

Welcome @lovinglyy, and thanks for the pull request! Is it possible to add a test for this condition?

[Trott]

This change appears to cause many existing http2 tests to fail.

=== release test-http2-client-socket-destroy ===Path: parallel/test-http2-client-socket-destroyMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.server.on.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-client-socket-destroy.js:18:31) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Http2Server.emit (events.js:182:13) at ServerHttp2Session.sessionOnStream (internal/http2/core.js:2516:19) at ServerHttp2Session.emit (events.js:182:13) at emit (internal/http2/core.js:232:8) at process._tickCallback (internal/process/next_tick.js:63:19)Command: out/Release/node --expose-internals /home/travis/build/nodejs/node/test/parallel/test-http2-client-socket-destroy.js=== release test-http2-compat-aborted ===Path: parallel/test-http2-compat-abortedMismatched <anonymous> function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-http2-compat-aborted.js:11:28) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Http2Server.emit (events.js:182:13) at Http2Server.onServerStream (internal/http2/compat.js:741:10) at Http2Server.emit (events.js:182:13) at ServerHttp2Session.sessionOnStream (internal/http2/core.js:2516:19) at ServerHttp2Session.emit (events.js:182:13) at emit (internal/http2/core.js:232:8)Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-http2-compat-aborted.js=== release test-http2-compat-errors ===Path: parallel/test-http2-compat-errorsMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-http2-compat-errors.js:18:28) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Http2Server.emit (events.js:182:13) at Http2Server.onServerStream (internal/http2/compat.js:741:10) at Http2Server.emit (events.js:182:13) at ServerHttp2Session.sessionOnStream (internal/http2/core.js:2516:19) at ServerHttp2Session.emit (events.js:182:13) at emit (internal/http2/core.js:232:8)Command: out/Release/node --expose-internals /home/travis/build/nodejs/node/test/parallel/test-http2-compat-errors.js=== release test-http2-max-concurrent-streams ===Path: parallel/test-http2-max-concurrent-streamsMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-max-concurrent-streams.js:45:30) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19) at Function.Module.runMain (internal/modules/cjs/loader.js:749:11) at startup (internal/bootstrap/node.js:270:19)Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-http2-max-concurrent-streams.js=== release test-http2-options-max-reserved-streams ===Path: parallel/test-http2-options-max-reserved-streamsMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at stream.pushStream.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-options-max-reserved-streams.js:38:39) at /home/travis/build/nodejs/node/test/common/index.js:350:15 at process._tickCallback (internal/process/next_tick.js:63:19)Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-http2-options-max-reserved-streams.js=== release test-http2-server-rst-stream ===Path: parallel/test-http2-server-rst-streamMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at tests.forEach (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:57:30) at Array.forEach (<anonymous>) at Http2Server.server.listen.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js:48:9) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Object.onceWrapper (events.js:273:13) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19)Command: out/Release/node /home/travis/build/nodejs/node/test/parallel/test-http2-server-rst-stream.js=== release test-http2-server-socket-destroy ===Path: parallel/test-http2-server-socket-destroyMismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.server.on.common.mustCall (/home/travis/build/nodejs/node/test/parallel/test-http2-server-socket-destroy.js:59:28) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Http2Server.emit (events.js:182:13) at emitListeningNT (net.js:1322:10) at process._tickCallback (internal/process/next_tick.js:63:19) at Function.Module.runMain (internal/modules/cjs/loader.js:749:11) at startup (internal/bootstrap/node.js:270:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:801:3)Mismatched noop function calls. Expected exactly 1, actual 0. at Object.mustCall (/home/travis/build/nodejs/node/test/common/index.js:310:10) at Http2Server.onStream (/home/travis/build/nodejs/node/test/parallel/test-http2-server-socket-destroy.js:31:31) at Http2Server.<anonymous> (/home/travis/build/nodejs/node/test/common/index.js:350:15) at Http2Server.emit (events.js:182:13) at ServerHttp2Session.sessionOnStream (internal/http2/core.js:2516:19) at ServerHttp2Session.emit (events.js:182:13) at emit (internal/http2/core.js:232:8) at process._tickCallback (internal/process/next_tick.js:63:19)Command: out/Release/node --expose-internals /home/travis/build/nodejs/node/test/parallel/test-http2-server-socket-destroy.js

[lovinglyy]

@Trott sorry, my bad with the tests, I did run the working ones I think, thanks for the welcome and actually running that tests, this time I did run it correctly, all the tests(got some failures but not http2 related or due the changes) I'll do a test with the issue reported.

[apapirovski]

That check is still not quite correct @lovinglyy (we just don't have any tests that would fail it, which probably indicates some gaps). I opened a PR with the correct fix in #22878

[lovinglyy]

Oh just noticed, @apapirovski, also did some tests to understand better, your pr does it definitely better as the ending property don't get a false value when aborted shouldn't be emitted, nice work there, however, shouldn't it still not be emitted if localClose is defined? If the stream truly aborts it is undefined.

[apapirovski]

however, shouldn't it still not be emitted if localClose is defined? If the stream truly aborts it is undefined.

If the two states are out of sync when they shouldn't be then it could indicate a deeper problem which I would be weary to mask. That is, I would consider the fact that we didn't call .end() on pushStreams by default a bug but had this fix been in place, it would mask the issue.

Since the user interface is in form of the writable stream, we should make sure the state of that writable stream represents the underlying state of the Http2Stream.

[lovinglyy]

Thank you for clarifying, now I understand how your pr makes much more sense I'll close this pr.