Skip to content

fix: implement full closing handshake#63

Merged
mccutchen merged 45 commits intomainfrom
closure
Oct 4, 2025
Merged

fix: implement full closing handshake#63
mccutchen merged 45 commits intomainfrom
closure

Conversation

@mccutchen
Copy link
Owner

@mccutchen mccutchen commented Oct 2, 2025

This somewhat messy change adds support for properly closing websocket connections via the closing handshake, where the peer that initiates a close waits for a reply before actually closing the TCP connection.

This fixes #47, which notes a mystery:

It seems like the autobahn test suite should probably cover this, but we're passing those tests with our incomplete implementation.

I now think that's because most/all of the closing scenarios tested by autobahn are for edge cases where it's appropriate for the server to immediately close the connection without waiting for a reply. As noted in a comment below, see section 7.1.7 of the RFC and search for "Fail the" in the RFC to see all the scenarios where immediately closing the connection is the correct/recommended/required behavior.

AFAICT the "full" closing handshake is mostly going to be useful if/when application code wants to signal errors.

@github-actions
Copy link

github-actions bot commented Oct 2, 2025

🔥 Run benchmarks comparing feb0d4a against main:

gh workflow run bench.yaml -f pr_number=63

Note: this comment will update with each new commit.

@mccutchen mccutchen added the enhancement New feature or request label Oct 2, 2025
@codecov
Copy link

codecov bot commented Oct 2, 2025

Codecov Report

❌ Patch coverage is 96.55172% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 96.68%. Comparing base (fe82f4e) to head (feb0d4a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
websocket.go 96.42% 2 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
+ Coverage   92.87%   96.68%   +3.81%     
==========================================
  Files           2        2              
  Lines         491      573      +82     
==========================================
+ Hits          456      554      +98     
+ Misses         28       14      -14     
+ Partials        7        5       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

github-actions bot commented Oct 2, 2025

benchstats: 13f5e8b...77719b1

View full benchmark output on the workflow summary.

goos: linux
goarch: amd64
pkg: github.com/mccutchen/websocket
cpu: AMD EPYC 7763 64-Core Processor                
                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt      │
                  │            sec/op            │   sec/op     vs base               │
ReadFrame/1KiB-4                     593.3n ± 4%   602.1n ± 5%       ~ (p=0.225 n=10)
ReadFrame/1MiB-4                     271.7µ ± 9%   296.5µ ± 4%  +9.13% (p=0.001 n=10)
WriteFrame/1KiB-4                    652.0n ± 1%   676.2n ± 2%  +3.70% (p=0.000 n=10)
WriteFrame/1MiB-4                    294.8µ ± 2%   309.6µ ± 2%  +5.02% (p=0.000 n=10)
geomean                              13.27µ        13.90µ       +4.80%

                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt       │
                  │             B/s              │     B/s       vs base               │
ReadFrame/1KiB-4                    1.622Gi ± 4%   1.598Gi ± 5%       ~ (p=0.218 n=10)
ReadFrame/1MiB-4                    3.595Gi ± 8%   3.294Gi ± 4%  -8.37% (p=0.001 n=10)
WriteFrame/1KiB-4                   1.475Gi ± 1%   1.423Gi ± 2%  -3.56% (p=0.000 n=10)
WriteFrame/1MiB-4                   3.313Gi ± 2%   3.154Gi ± 2%  -4.78% (p=0.000 n=10)
geomean                             2.310Gi        2.205Gi       -4.57%

                  │ ./baseline/bench-results.txt │       ./head/bench-results.txt        │
                  │             B/op             │     B/op      vs base                 │
ReadFrame/1KiB-4                    1.164Ki ± 0%   1.164Ki ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                    1.008Mi ± 0%   1.008Mi ± 0%       ~ (p=0.261 n=10)
WriteFrame/1KiB-4                   1.125Ki ± 0%   1.125Ki ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1MiB-4                   1.008Mi ± 0%   1.008Mi ± 0%       ~ (p=0.474 n=10)
geomean                             34.37Ki        34.37Ki       -0.00%
¹ all samples are equal

                  │ ./baseline/bench-results.txt │      ./head/bench-results.txt       │
                  │          allocs/op           │ allocs/op   vs base                 │
ReadFrame/1KiB-4                      5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
ReadFrame/1MiB-4                      5.000 ± 0%   5.000 ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1KiB-4                     1.000 ± 0%   1.000 ± 0%       ~ (p=1.000 n=10) ¹
WriteFrame/1MiB-4                     1.000 ± 0%   1.000 ± 0%       ~ (p=1.000 n=10) ¹
geomean                               2.236        2.236       +0.00%
¹ all samples are equal

@mccutchen mccutchen merged commit 1c1c8fa into main Oct 4, 2025
21 of 23 checks passed
@mccutchen mccutchen deleted the closure branch October 4, 2025 11:27
mccutchen added a commit that referenced this pull request Oct 5, 2025
Slightly more thorough tests to complement #63.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: properly handle full closing handshake

1 participant