Repository navigation
feat: add WebSocket application support for Jetty (closes #134) - #162
Conversation
- Add WebSocketHandler and WebSocketSession interfaces to enkan-web - Add jetty-ee10-websocket-jetty-server dependency to enkan-component-jetty - Implement JettyWebSocketEndpoint (Session.Listener.AbstractAutoDemanding bridge) and JettyWebSocketSession (Enkan WebSocketSession backed by Jetty Session) - Add JettyComponent.addWebSocket(path, handler) for endpoint registration - Wire WebSocket container via JettyWebSocketServletContainerInitializer in JettyAdapter Undertow support and RFC 8441 are deferred to a follow-up issue. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Route sendText/sendBinary failures to WebSocketHandler.onError via Callback.from instead of swallowing errors with Callback.NOOP - Copy ByteBuffer payload before callback.succeed() in onWebSocketBinary to prevent use-after-recycle when Jetty reuses the underlying buffer - Guard addWebSocket() to throw MisconfigurationException if called after the server has started - Mark JettyWebSocketEndpoint.session field volatile for safe cross-thread visibility - Add JettyWebSocketEndpointTest (10 tests) and expand JettyComponentTest with 3 addWebSocket behaviour tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove unused ByteBuffer import from JettyComponentTest - Simplify onWebSocketBinary copy: replace payload.duplicate() with payload directly (duplicate() was unnecessary since payload is not referenced after the copy) - Narrow @SuppressWarnings("unchecked") from method level to the single cast declaration in JettyAdapter.runJetty - Document Jetty path-mapping syntax (exact vs wildcard) in JettyComponent.addWebSocket Javadoc Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds first-class, server-agnostic WebSocket handler/session APIs to enkan-web and wires Jetty 12 (EE10) WebSocket endpoint registration through JettyComponent/JettyAdapter, with unit tests for the Jetty bridge.
Changes:
- Introduce
WebSocketHandlerandWebSocketSessioninterfaces inenkan-web. - Add Jetty EE10 WebSocket server dependency and register WS mappings during Jetty startup.
- Implement Jetty→Enkan bridge classes (
JettyWebSocketEndpoint,JettyWebSocketSession,JettyWebSocketCreatorFactory) and add unit tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| enkan-web/src/main/java/enkan/web/websocket/WebSocketSession.java | Defines server-agnostic session API for sending/closing WebSockets. |
| enkan-web/src/main/java/enkan/web/websocket/WebSocketHandler.java | Defines lifecycle callback interface for app-level WebSocket handling. |
| enkan-component-jetty/pom.xml | Adds Jetty EE10 WebSocket server dependency needed for endpoint registration. |
| enkan-component-jetty/src/main/java/enkan/component/jetty/JettyComponent.java | Adds addWebSocket registration API and passes handlers into Jetty startup options. |
| enkan-component-jetty/src/main/java/enkan/adapter/JettyAdapter.java | Registers WebSocket mappings via JettyWebSocketServletContainerInitializer. |
| enkan-component-jetty/src/main/java/enkan/adapter/websocket/JettyWebSocketSession.java | Implements WebSocketSession over Jetty’s Session. |
| enkan-component-jetty/src/main/java/enkan/adapter/websocket/JettyWebSocketEndpoint.java | Bridges Jetty frame/lifecycle events to WebSocketHandler. |
| enkan-component-jetty/src/main/java/enkan/adapter/websocket/JettyWebSocketCreatorFactory.java | Exposes a public creator factory while keeping endpoint/session package-private. |
| enkan-component-jetty/src/test/java/enkan/component/jetty/JettyComponentTest.java | Tests addWebSocket registration, “before start” enforcement, and chaining. |
| enkan-component-jetty/src/test/java/enkan/adapter/websocket/JettyWebSocketEndpointTest.java | Tests event dispatching and error routing behavior for the Jetty bridge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public JettyComponent addWebSocket(String path, WebSocketHandler handler) { | ||
| if (server != null) { | ||
| throw new MisconfigurationException("web.WEBSOCKET_MUST_REGISTER_BEFORE_START", path); | ||
| } | ||
| wsHandlers.put(path, handler); |
There was a problem hiding this comment.
New MisconfigurationException code web.WEBSOCKET_MUST_REGISTER_BEFORE_START is introduced here, but there are no corresponding web.WEBSOCKET_MUST_REGISTER_BEFORE_START.problem/solution entries in the misconfiguration*.properties resources (at least in enkan-web). This will surface as "(message not found)" at runtime; please add message keys for this code (and any locale variants you support).
| // Copy the payload before calling callback.succeed() — Jetty may recycle | ||
| // the underlying buffer once the callback completes. | ||
| ByteBuffer copy = ByteBuffer.allocate(payload.remaining()); | ||
| copy.put(payload); | ||
| copy.flip(); | ||
| handler.onBinary(session, copy); | ||
| callback.succeed(); |
There was a problem hiding this comment.
onWebSocketBinary must always complete the Jetty Callback. If handler.onBinary(...) throws, callback.succeed() is never invoked, which can stall Jetty's flow control/backpressure handling for the connection. Wrap handler dispatch in try/catch/finally and call callback.fail(...) (and route the exception to handler.onError(...)) to guarantee callback completion.
| // Copy the payload before calling callback.succeed() — Jetty may recycle | |
| // the underlying buffer once the callback completes. | |
| ByteBuffer copy = ByteBuffer.allocate(payload.remaining()); | |
| copy.put(payload); | |
| copy.flip(); | |
| handler.onBinary(session, copy); | |
| callback.succeed(); | |
| // Copy the payload before completing the callback — Jetty may recycle | |
| // the underlying buffer once the callback completes. | |
| ByteBuffer copy = ByteBuffer.allocate(payload.remaining()); | |
| copy.put(payload); | |
| copy.flip(); | |
| try { | |
| handler.onBinary(session, copy); | |
| callback.succeed(); | |
| } catch (Throwable cause) { | |
| handler.onError(session, cause); | |
| callback.fail(cause); | |
| } |
| /** | ||
| * Called when an error occurs on the connection. | ||
| * | ||
| * <p>The connection will be closed after this callback returns. | ||
| * | ||
| * @param session the affected session | ||
| * @param cause the error | ||
| */ | ||
| void onError(WebSocketSession session, Throwable cause); |
There was a problem hiding this comment.
The onError Javadoc says "The connection will be closed after this callback returns", but WebSocketHandler is server-agnostic and this guarantee isn't enforced by the interface (and may not hold across adapters). Consider rewording to avoid promising a close semantics that implementations might not provide (e.g., "the server implementation may close the connection").
| /** | ||
| * Registers a WebSocket handler at the given path. | ||
| * | ||
| * <p>Must be called before the component is started. | ||
| * Multiple paths can be registered by chaining calls. | ||
| * | ||
| * <p>Example: | ||
| * <pre>{@code | ||
| * new JettyComponent() | ||
| * .addWebSocket("/ws/chat", new ChatHandler()) | ||
| * .addWebSocket("/ws/notify", new NotifyHandler()) | ||
| * }</pre> |
There was a problem hiding this comment.
This PR is marked "closes #134", but the linked issue’s acceptance criteria includes Undertow endpoint registration and an integration test; neither appears in this change set. Consider adjusting the PR wording (don’t auto-close the issue) or updating/splitting the issue so remaining work isn’t lost when this merges.
| * | ||
| * @param data the binary payload |
There was a problem hiding this comment.
sendBinary(ByteBuffer data) is documented as asynchronous but doesn’t define buffer ownership/lifetime (whether the implementation copies data, or whether callers must keep it unmodified until the send completes). Because ByteBuffer is mutable and position/limit can change, this should be specified explicitly (or changed to a safer type like byte[]) to avoid hard-to-debug data corruption in async sends.
| * | |
| * @param data the binary payload | |
| * Implementations must make a defensive copy of the bytes remaining | |
| * between {@code data.position()} and {@code data.limit()} before this | |
| * method returns, so callers may safely reuse or modify the provided | |
| * buffer after the call returns. | |
| * | |
| * @param data the binary payload; the sent bytes are the remaining bytes | |
| * in the buffer at the time of the call |
| public JettyComponent addWebSocket(String path, WebSocketHandler handler) { | ||
| if (server != null) { | ||
| throw new MisconfigurationException("web.WEBSOCKET_MUST_REGISTER_BEFORE_START", path); | ||
| } |
There was a problem hiding this comment.
addWebSocket accepts null for path and/or handler and will store it in the map; this will later fail with a less actionable NPE/ClassCast at startup or when Jetty tries to add the mapping. Consider validating inputs up front (e.g., reject null/blank path and null handler with a MisconfigurationException or Objects.requireNonNull).
| } | |
| } | |
| if (path == null || path.trim().isEmpty()) { | |
| throw new MisconfigurationException("web.WEBSOCKET_PATH_REQUIRED", path); | |
| } | |
| if (handler == null) { | |
| throw new MisconfigurationException("web.WEBSOCKET_HANDLER_REQUIRED", path); | |
| } |
- Add misconfiguration.properties (en/ja) to enkan-component-jetty with message keys for WEBSOCKET_MUST_REGISTER_BEFORE_START, WEBSOCKET_PATH_REQUIRED, WEBSOCKET_HANDLER_REQUIRED - Guard addWebSocket() against null/blank path and null handler; throw MisconfigurationException with actionable keys instead of NPE at startup - Wrap handler.onBinary dispatch in try/catch so callback.fail() is guaranteed when the handler throws, preventing Jetty flow-control stalls - Soften onError Javadoc: connection-close after onError is server-specific, not guaranteed by the interface - Document ByteBuffer ownership contract on sendBinary: implementations must copy remaining bytes before returning - Add tests: null/blank path, null handler, onBinary handler exception (verifies both onError routing and callback.fail invocation) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- JettyWebSocketSession.sendBinary now copies the ByteBuffer before passing it to Jetty's async API, fulfilling the contract documented in WebSocketSession.sendBinary Javadoc (callers may safely reuse the buffer after the call returns) - Reorder addWebSocket guards: validate path/handler arguments first, then check lifecycle state; previously a null path after start would throw WEBSOCKET_MUST_REGISTER_BEFORE_START (misleading) instead of WEBSOCKET_PATH_REQUIRED - Add sendBinaryDoesNotExposeOriginalBufferToJetty test to pin the copy semantics Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package enkan.web.websocket; | ||
|
|
||
| import java.nio.ByteBuffer; |
There was a problem hiding this comment.
enkan-web is a named module (has module-info.java), but enkan.web.websocket is not currently exported. Since WebSocketHandler/WebSocketSession are intended as public API and are referenced from other modules (e.g., enkan-component-jetty), add exports enkan.web.websocket; to enkan-web/src/main/java/module-info.java or consumers will fail to compile/access these types under JPMS.
| @Override | ||
| public void onWebSocketOpen(Session jettySession) { | ||
| super.onWebSocketOpen(jettySession); | ||
| this.session = new JettyWebSocketSession(id, jettySession, handler); | ||
| handler.onOpen(session); | ||
| } | ||
|
|
||
| @Override | ||
| public void onWebSocketText(String message) { | ||
| handler.onMessage(session, message); | ||
| } | ||
|
|
||
| @Override | ||
| public void onWebSocketBinary(ByteBuffer payload, Callback callback) { | ||
| // Copy the payload before completing the callback — Jetty may recycle | ||
| // the underlying buffer once the callback completes. | ||
| ByteBuffer copy = ByteBuffer.allocate(payload.remaining()); | ||
| copy.put(payload); | ||
| copy.flip(); | ||
| try { | ||
| handler.onBinary(session, copy); | ||
| callback.succeed(); | ||
| } catch (Throwable cause) { | ||
| handler.onError(session, cause); | ||
| callback.fail(cause); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onWebSocketClose(int statusCode, String reason) { | ||
| handler.onClose(session, statusCode, reason); | ||
| } |
There was a problem hiding this comment.
JettyWebSocketEndpoint only wraps onWebSocketBinary in a try/catch. If the application handler throws from onWebSocketOpen (handler onOpen), onWebSocketText, or onWebSocketClose, the exception will propagate into Jetty and may terminate the connection without delivering WebSocketHandler#onError. Consider consistently catching Throwable around these handler invocations and routing to handler.onError(...) (and failing/closing as appropriate).
| public static JettyWebSocketCreator forHandler(WebSocketHandler handler) { | ||
| return (req, resp) -> new JettyWebSocketEndpoint(UUID.randomUUID().toString(), handler); | ||
| } |
There was a problem hiding this comment.
JettyWebSocketCreatorFactory.forHandler(...) is public but doesn't validate handler. Passing null will produce an endpoint with a null handler and cause a delayed NullPointerException on first event. Add an explicit null-check here and throw a clear exception early.
| public JettyComponent addWebSocket(String path, WebSocketHandler handler) { | ||
| if (path == null || path.isBlank()) { | ||
| throw new MisconfigurationException("web.WEBSOCKET_PATH_REQUIRED", path); | ||
| } | ||
| if (handler == null) { | ||
| throw new MisconfigurationException("web.WEBSOCKET_HANDLER_REQUIRED", path); | ||
| } | ||
| if (server != null) { | ||
| throw new MisconfigurationException("web.WEBSOCKET_MUST_REGISTER_BEFORE_START", path); | ||
| } | ||
| wsHandlers.put(path, handler); | ||
| return this; | ||
| } |
There was a problem hiding this comment.
addWebSocket only blocks registration when server != null, but server is assigned after runJetty(...) returns. If addWebSocket(...) is called concurrently while start() is in progress, it can mutate wsHandlers while JettyAdapter is iterating it, risking ConcurrentModificationException and/or missing mappings. Consider preventing registration during startup (e.g., set a starting flag before runJetty) and/or passing an immutable snapshot of wsHandlers into options (e.g., Map.copyOf(...)).
- Export enkan.web.websocket from enkan-web module-info.java so consumers can access WebSocketHandler/WebSocketSession under JPMS - Wrap onOpen/onMessage/onClose handler calls in try/catch and route exceptions to handler.onError, consistent with onBinary behaviour - Null-check handler in JettyWebSocketCreatorFactory.forHandler to fail fast at registration rather than on first event - Pass Map.copyOf(wsHandlers) to options in JettyComponent.start() to prevent ConcurrentModificationException if addWebSocket is called concurrently while startup is in progress - Add tests for onOpen/onMessage/onClose handler exception routing Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
Design notes
Scope note
This PR implements Phase 1 (Jetty only) of issue #134. Remaining acceptance criteria (Undertow support, integration test) are tracked separately. Issue #134 remains open until all phases are complete.
Test plan
Related: #134