Skip to content

refactor(client): speak WebSocket through HttpClient instead of a hand-rolled framer - #193

Merged
magnusp merged 4 commits into
mainfrom
feat/i192-apache-ws
Oct 24, 2026
Merged

magnusp merged 4 commits into
mainfrom
feat/i192-apache-ws

Conversation

@magnusp

@magnusp magnusp commented Oct 24, 2026 •

Copy link
Copy Markdown
Owner

Replace the hand-rolled RFC 6455 transport inside LibsqlWebSocketClient with Apache
HttpClient's httpclient5-websocket module. The WebSocket transport now reuses a library
that already does framing, handshake, TLS, and fragmentation, and about 380 lines of that
code leave the client.

Fixes #192.

Why

sqld answers the upgrade with a 101 that carries content-length: 31 and no body, so the
JDK client's java.net.http.WebSocket never finishes its opening handshake. #190 shipped a
hand-written framer to work around that. HttpClient treats a 101 as informational and
upgrades without reading a body, so the workaround is no longer needed.

What changes

  • libsql-client depends on httpclient5-websocket 5.7-beta1, pinned in the root pom.
    LibsqlWebSocketClient keeps its request_id matching, stream ids, and blocking call
    semantics on top of the library.
  • The Transport inner class, the static handshake, the frame encoder and decoder, the
    accept-key computation, and the socket and SSLSocket setup are deleted.
  • Closing a connection tears the client down in one step, so close latency and thread
    lifetime stay what the hand-rolled transport gave: single-digit milliseconds and no
    leftover IO threads.
  • Both WebSocket integration tests now run every case over ws:// and wss://. The wss
    path goes through an nginx TLS terminator in front of sqld with a self-signed localhost
    certificate that the failsafe JVM trusts.

Scope

Tradeoffs

  • 5.7 is beta, so the version is pinned exactly and the adapter stays inside one class.
  • The dependency brings httpclient5, httpcore5, httpcore5-h2, httpclient5-cache,
    and slf4j-api into libsql-client. An application with no SLF4J binding gets SLF4J's
    one-time "no providers" notice on stderr.
  • The hand-rolled transport is deleted rather than kept behind a flag, on your call.

Blast Radius

Every jdbc:libsql:ws:// and wss:// connection changes transport. The HTTP transport,
the codec, and the JDBC layer are untouched, and their suites still pass. The hidden cost
is dependency footprint for anyone who only uses http:// URLs but resolves the client
module's transitive dependencies.

Verification

  • ./mvnw spotless:check passes, and ./mvnw test passes (374 JDBC tests, 35 codec tests).
  • WebSocketTransportIT and WebSocketProtobufTransportIT run 15 cases across ws:// and
    wss:// against ghcr.io/tursodatabase/libsql-server:v0.24.33 and pass.
  • mvn clean verify passes locally, including the HTTP integration tests, the Hikari pool
    churn test, the toxiproxy test, and the conformance suite.

Pinned exactly: 5.7 is a beta, so an upgrade must be a deliberate edit.
…d-rolled framer

sqld answers the upgrade with a 101 that carries content-length: 31 and no body, which
the JDK client waits on forever. HttpClient treats a 101 as informational, so it upgrades
without a body and the custom RFC 6455 framing can go.

The request_id matching, stream ids, blocking call semantics, and SQLITE_BUSY handling stay
in the client. permessage-deflate is off so the wire bytes do not change.
A TLS terminator in front of sqld exercises the real handshake, so the wss path is no
longer untested. Table names carry the scheme because both runs share one sqld instance.
@magnusp

magnusp commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Independent verification — VERDICT: FAIL

Verified head 5a7de6cbc721494cdee8fcb9ba9c6dd668b6d9f4 in a fresh worktree. The transport swap itself works and both suites pass, but Connection.close() regressed: every ws/wss close blocks 5,000 ms and permanently leaks 3 daemon threads per connection. That contradicts "behavior is otherwise unchanged", and it is unbounded for a pool that churns connections.

What I ran

$ mise exec -- ./mvnw -B -s /tmp/mvn-proxy-settings.xml spotless:check
[INFO] BUILD SUCCESS   (0.98 s, all 6 modules)

$ DOCKER_CONFIG=.../libsql-conformance/src/test/docker-config mise exec -- ./mvnw -B -s ... \
    -pl libsql-itests -am -Dit.test='WebSocketTransportIT,WebSocketProtobufTransportIT' verify
[INFO] Running com.github.magnusp.libsql.itest.WebSocketTransportIT
[INFO] Running com.github.magnusp.libsql.itest.WebSocketProtobufTransportIT
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS   (Total time: 01:20 min)

The -am build also ran the unit suites: 374 JDBC tests and 35 codec tests, all green. The transport logs show Sec-WebSocket-Protocol: hrana3 and hrana3-protobuf upgrades over both schemes, and nginx serves wss:// on 8443.

Diff checks: no Transport, sendFrame, acceptKey, SSLSocket, or MAX_FRAME_BYTES left (grep returns none), and nothing else references the deleted internals. httpclient5.websocket.version is pinned exactly to 5.7-beta1 in the root pom.

Probes (all via plain JDBC against ghcr.io/tursodatabase/libsql-server:v0.24.33 on a separate port)

1. Close/abandon timing and threads (/tmp/probe193/ChurnProbe.java, N open→SELECT→close cycles, then a 10 s settle):

close[0]=5000ms websocket-main_threads=2
close[1]=5000ms websocket-main_threads=4
close[2]=5000ms websocket-main_threads=6
baseline_threads=6 after_threads=15
websocket_main_before=0 after=6
total_close_ms=15000 avg=5000
thread_names=[... main(NON-DAEMON), requester-dispatch-1(d), requester-dispatch-1(d), requester-dispatch-1(d),
              websocket-main-1(d) x3, websocket-main-2(d) x3]

The same probe on the PR's base commit (6591155) shows close[0..2]=0ms, no websocket-main* threads, and 0 extra threads. So both symptoms are introduced here. The threads are daemon, so the JVM still exits; the leak is per live process.

Cause: shutdown() calls socket.close() then client.initiateShutdown() + client.awaitShutdown(SHUTDOWN_TIMEOUT), which never calls CloseableWebSocketClient.close(...). The reactor/connection pool and the fixed executor are therefore never torn down; awaitShutdown simply burns the 5 s timeout. I confirmed this by compiling an unmodified copy of the class in /tmp with only that call changed to client.close(CloseMode.IMMEDIATE):

close[0]=3ms websocket-main_threads=0
close[1]=3ms websocket-main_threads=1
close[2]=0ms websocket-main_threads=2
after_threads=6   websocket_main_before=0 after=0

That is the one-line fix: replace initiateShutdown()/awaitShutdown(...) with client.close(...) (or call it in a finally), and keep the 5 s await only as a guard if desired. awaitShutdown alone does not close the pool.

2. Large single messages (LargeResultProbe): SELECT randomblob(4000000) succeeds over JSON (got=4000000) and over ?encoding=protobuf (LARGE_OK). 8 MB is rejected by sqld before the wire (SQLException: SQLite error: string or blob too big), so the client's 1 GiB frame/message limits are not the bottleneck. The failure maps cleanly, no hang.

3. Server death / closed socket (KillServerProbe): connected, queried, stopped the sqld container under it, then queried again:

first_query=1
mapped_sql_exception_ms=0 class=java.sql.SQLException msg=Hrana WebSocket is closed
KILL_PROBE_DONE   (JVM exit 0, no hang)

Error mapping is fine.

Notes for a maintainer

  • The 5 s close is also paid on failed connects (the constructor's catch calls the same shutdown()), so a bad host costs the connect timeout plus 5 s.
  • Minor: dispatch()'s decode-error path no longer tears the socket down (the old transport.abort() is gone), so a malformed frame fails pending requests but leaves the socket idle until GC/peer close.
  • Test-only: javax.net.ssl.trustStore is set for the whole failsafe JVM, so every IT in libsql-itests trusts only the self-signed test cert. Fine today (all other ITs are plain HTTP, image pulls happen in the Docker daemon), but it will bite the first HTTPS test added to that module.

Probes live in /tmp/probe193 (untracked, outside the repo); I did not modify the branch, commit, or push.

…eactor that stays up

initiateShutdown() only asks the reactor to stop; the executor and connection pool keep
running, so awaitShutdown() burned its full timeout and left three threads per connection
behind. Closing the client stops the reactor, the executor, and the pool in one step, so
connection close returns in single-digit milliseconds again.

A decode failure now closes the socket, as the hand-rolled transport did.
@magnusp

magnusp commented Oct 24, 2026

Copy link
Copy Markdown
Owner Author

Re-verification — VERDICT: PASS+NOTES

Verified head e63c539cd4676509c154801fb516bdabbfe27118 in a fresh worktree, built from scratch (no reused target dirs). git diff 5a7de6c..e63c539 is exactly the one file / 6 added / 10 removed the commit message describes, and e63c539 is a descendant of the head I verified before.

The regression I reported is gone: close latency is back to single-digit milliseconds, the three-threads-per-connection leak is gone, rollback-on-close still works, and the close frame still goes out.

Regression, before → after (same probes, same server, fresh builds)

measurement 5a7de6c (before) e63c539 (after)
Connection.close() 5,000 ms every time 2–3 ms
threads after N closes +3 per connection, still alive after 10 s (websocket-main-1/-2, requester-dispatch-1) back to baseline 6, zero websocket-main*
WebSocketTransportIT + WebSocketProtobufTransportIT 01:20 min 10.7 s

Commands run

$ git worktree add -f /tmp/wt-193-verify2 e63c539        # fresh checkout, clean
$ mise exec -- ./mvnw -B -s /tmp/mvn-proxy-settings.xml spotless:check
[INFO] BUILD SUCCESS   (1.4 s, 6 modules)

$ DOCKER_CONFIG=.../libsql-conformance/src/test/docker-config mise exec -- ./mvnw -B -s ... \
    -pl libsql-itests -am -Dit.test='WebSocketTransportIT,WebSocketProtobufTransportIT' verify
[INFO] Tests run: 374, Failures: 0, Errors: 0, Skipped: 0   (jdbc unit, from -am)
[INFO] Tests run: 35, Failures: 0, Errors: 0, Skipped: 0    (codec unit, from -am)
[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0    (ws + wss integration)
[INFO] BUILD SUCCESS   (Total time: 10.666 s)

Probe classpath was rebuilt from /tmp/wt-193-verify2/libsql-{jdbc,client,hrana-codec}/target/classes plus a fresh dependency:build-classpath (it does include httpclient5-websocket/httpclient5 5.7-beta1 and httpcore5 5.5-beta4, so nothing had to be added by hand).

Probes

Close latency / thread count (CloseProbe, ChurnProbeLong with 2 open→SELECT→close cycles and a 10 s settle):

query=42
close_ms=2 total_ms=265
threads_before_close=32 threads_after_close=6
second_close_ms=0

close[0]=3ms websocket-main_threads=0
close[1]=2ms websocket-main_threads=2
baseline_threads=6 after_threads=6
websocket_main_before=0 after=0
total_close_ms=5 avg=2
thread_names=[Common-Cleaner(d), Finalizer(d), Notification Thread(d), Reference Handler(d),
              Signal Dispatcher(d), main(NON-DAEMON), ]

(The transient websocket-main_threads=2 is the async pool shutdown right after close() returns; nothing remains after the settle.)

Does close still send a close frame? A logging TCP proxy between driver and sqld (CloseFrameProbe), parsing client→server frames, 3 consecutive runs:

client_frames=[op=1,...,len=27, op=1,...,len=80, op=1,...,len=119, op=1,...,len=81,
               op=8,fin=true,masked=true,len=9,code=1000]
close_frame_sent=true      # 3/3 runs, code 1000, reason "closing" = 9-byte payload

So socket.close(CLOSE_NORMAL, "closing") is actually flushed to the wire before client.close(CloseMode.IMMEDIATE) returns.

Close mid-transaction, close twice, close idle (MidTxProbe):

inserted_uncommitted=1
close_mid_tx_ms=3
second_close_ms=0 threw=false
rows_after_reconnect=0
close_idle_ms=0 threw=false
second_close_idle=ok

Uncommitted work is rolled back, a second close() is a no-op, and neither path throws out of close().

Close after the server is gone (KillServerProbe, container stopped under a live connection):

mapped_sql_exception_ms=1 class=java.sql.SQLException msg=Hrana WebSocket is closed
close_after_server_dead_ms=1 threw=false
KILL_PROBE_DONE   (JVM exit 0)

Large messages (unchanged, re-run on this build): randomblob(4000000) returns 4,000,000 bytes over JSON and over ?encoding=protobuf; 8 MB is still rejected server-side by SQLite, not by the client.

Notes (non-blocking)

  • The close frame is queued and then the client is torn down immediately, so delivery depends on the frame being flushed before teardown. It was delivered in 3/3 observed runs, but this is scheduling-dependent by construction; harmless for sqld, which never echoes the close anyway. If you want it guaranteed, flushing/waiting on the close future before CloseMode.IMMEDIATE would make it deterministic.
  • socket is a plain (non-volatile) field assigned after connect() returns. The new decode-failure socket.close(...) would NPE if a malformed frame arrived before that assignment, i.e. between upgrade and the first Hello response. Not observed and not reachable with a well-behaved Hrana server, but it is a wider window than the old transport.abort() had.
  • Carried over from the parent PR, still test-only: javax.net.ssl.trustStore is set for the whole failsafe JVM, so every IT in libsql-itests trusts only the self-signed test certificate.

No code changes, commits, or pushes; probes live in /tmp/probe193 and the only container I started was on port 19094, now stopped.

@magnusp
magnusp merged commit 404bba6 into main Oct 24, 2026
6 checks passed
@magnusp
magnusp deleted the feat/i192-apache-ws branch October 24, 2026 08:00
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.

WebSocket: replace hand-rolled transport with Apache HttpClient 5.7 (JDK client workaround)

1 participant