Skip to content

fix: shutdown and stream lifecycle handling#83

Merged
richard-ramos merged 5 commits into
mainfrom
fix/various-3
Apr 20, 2026
Merged

fix: shutdown and stream lifecycle handling#83
richard-ramos merged 5 commits into
mainfrom
fix/various-3

Conversation

@richard-ramos

Copy link
Copy Markdown
Member

Improves QUIC context shutdown safety and stream lifecycle behavior.

  • Added explicit QUIC context running state.
  • Split context shutdown from native resource destruction.
  • Ignored late datagrams after context shutdown starts.
  • Guarded engine operations when the context is no longer running.
  • Fixed pending stream cleanup on server connection close.
  • Improved stream EOF, blocked read, write, and flush error handling.
  • Released GC refs when client dial creation fails.
  • Fixed certificate DER conversion for nil/invalid X509 values.
  • Added lifecycle and TLS coverage for the new shutdown and stream edge cases.

@richard-ramos
richard-ramos requested a review from a team as a code owner April 20, 2026 13:53
@richard-ramos
richard-ramos requested review from Copilot, gmelodie and vladopajic and removed request for a team and Copilot April 20, 2026 13:53

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens QUIC shutdown semantics and stream lifecycle behavior by introducing an explicit “running” state for contexts, splitting quiesce vs native teardown, and tightening error/edge-case handling (streams, connections, TLS cert conversion), with added tests covering the new behavior.

Changes:

  • Introduces QuicContext.running with stop() (quiesce) and destroy() (native teardown), and guards engine operations/packet ingestion when not running.
  • Improves stream/connection lifecycle correctness: pending stream cancellation on server close, stronger stream write/flush/wantwrite/wantread error handling, and safer client dial GC ref cleanup.
  • Fixes/extends TLS certificate DER conversion behavior (nil/invalid handling) and adds lifecycle/TLS tests for shutdown + late datagram cases.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lsquic/context/context.nim Adds running state, guards engine operations, and splits stop() from destroy().
lsquic/context/io.nim Ignores datagrams when context is not running.
lsquic/client.nim Updates client shutdown flow to stop() contexts before UDP close and destroy() after.
lsquic/server.nim Updates listener shutdown ordering and fixes log message typo.
lsquic/context/client.nim Ensures running=true and releases GC refs when dial fails.
lsquic/context/server.nim Ensures running=true and cancels pending streams on server connection close.
lsquic/connection.nim Processes engine after stream creation request to advance pending stream lifecycle.
lsquic/stream.nim Tightens close/write error handling (flush/wantwrite) and fixes close typo.
lsquic/context/stream.nim Improves blocked-read handling via wantread management and fixes log message.
lsquic/certificates.nim Rejects nil X509 and ensures DER conversion only succeeds on positive length.
tests/test_lifecycle.nim Adds coverage for late datagrams after context stop/destroy.
tests/test_tlsconfig.nim Adds coverage for DER conversion success and nil rejection.
lsquic/helpers/transportaddr.nim Fixes assertion message typo.
.github/workflows/lint.yml Bumps NPH action version used in lint workflow.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lsquic/context/context.nim Outdated
Comment thread lsquic/context/context.nim Outdated
Copilot AI review requested due to automatic review settings April 20, 2026 18:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lsquic/context/server.nim
@richard-ramos
richard-ramos merged commit 6f87af4 into main Apr 20, 2026
16 checks passed
@richard-ramos
richard-ramos deleted the fix/various-3 branch April 23, 2026 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants