Repository navigation
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
http2
inoway46
marked this pull request as ready for review
November 3, 2026 07:03
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #67215 +/- ##
==========================================
- Coverage 90.71% 90.67% -0.05%
==========================================
Files 797 797
Lines 278898 279370 +472
Branches 53627 53739 +112
==========================================
+ Hits 252995 253309 +314
- Misses 16269 16356 +87
- Partials 9634 9705 +71
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
mcollina
requested changes
Nov 4, 2026
| session.destroy(error); | ||
| // TLS errors reported by the native handle leave established sockets open. | ||
| // Cancel writes that cannot complete after a fatal TLS error. | ||
| if (this instanceof tls.TLSSocket && this._hadError) |
Member
There was a problem hiding this comment.
This looks like we have a bug somewhere else, it should not be need a special if here
Contributor
Author
There was a problem hiding this comment.
Thanks, I reproduced this with TLS alone and moved the fix there. TLS should close an unusable connection without relying on HTTP/2.
destroy(err) alone fixes the original issue. I used _closeAfterHandlingError to preserve connection details in error handlers, but that exposed an existing getProtocol() crash in close handlers. The first commit fixes that; the second adds the TLS cleanup.
inoway46
changed the title
inoway46
marked this pull request as draft
November 4, 2026 13:24
Clear a deferred socket handle before invoking close listeners. TLS has already freed its SSL state at this point, so retaining the handle lets getProtocol() access a null SSL pointer and crash the process. Keep connection details available during tlsClientError and add coverage for querying the protocol after a failed handshake closes the socket. Signed-off-by: inoway46 <inoueyuya416@gmail.com> Assisted-by: Codex
Destroy established TLS sockets on fatal errors so pending writes cannot prevent HTTP/2 session cleanup from completing. Defer closing until after error handling to preserve connection details for listeners. Add regression coverage for standalone TLS socket closure and HTTP/2 cleanup with default and custom TLS connections. Fixes: nodejs#67213 Signed-off-by: inoway46 <inoueyuya416@gmail.com> Assisted-by: Codex
inoway46
force-pushed
the
fix/http2-tls-fatal-cleanup
branch
from
November 4, 2026 13:30
5aaccbd to
87ce55b
Compare
inoway46
added
the
commit-queue-rebase
inoway46
marked this pull request as ready for review
November 4, 2026 13:32
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Destroy established TLS sockets on fatal TLS errors so pending writes cannot block HTTP/2 session cleanup. Preserve connection details during error handling using the existing
_closeAfterHandlingErrormechanism.This requires fixing an existing crash in that mechanism: TLS frees its SSL state before emitting
close, but the socket still exposes the handle to close listeners. CallinggetProtocol()from a close listener after a failed handshake can therefore crash the process.The first commit clears the closed handle before emitting
closeand adds a regression test. The second applies the TLS cleanup change and adds coverage for standalone TLS closure and HTTP/2 cleanup with default and custom connections.Fixes: #67213