Repository navigation
Conversation
- Implement close_output/2 to send close frame while continuing to receive - Add output_closed_loop to process incoming frames after output is closed - Handle callback replies gracefully when output is closed (ignore sends) - Add error messages for closing and output_closed connection states Implements dominicletz#140
Summary of ChangesHello @chgeuer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances WebSockex by adding support for the WebSocket half-close pattern. This feature, analogous to TCP's shutdown(SHUT_WR), allows one endpoint to signal it has finished sending data while remaining open to receive data from the other side. This is particularly useful for protocols like Azure Relay's HybridConnection, which require this behavior for efficient TCP connection forwarding over WebSockets. The implementation includes a new public API function, a dedicated internal loop for the half-closed state, and robust error handling, all backed by new test cases. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for WebSocket half-close functionality by adding a close_output/2 function. The implementation is well-documented and includes relevant tests. The overall approach is sound, creating a new state loop (output_closed_loop) to handle receiving frames after the client has signaled it is done sending.
My review includes two main points. First, a high-severity issue regarding an unhandled error case that could cause the process to crash if close_output is called with an invalid close code. Second, a medium-severity suggestion to refactor the newly introduced code to reduce significant duplication between the new output_closed_loop and the existing websocket_loop, which would improve long-term maintainability.
Add catch-all {:error, reason} clause to handle errors like
FrameEncodeError when an invalid close code is provided.
This prevents process crashes and returns the error to the caller,
matching the pattern used in sync_send/5.
Reduce code duplication by using a single unified loop with an output_closed_reason field in state to track the half-close state. This removes ~160 lines of duplicated code including: - output_closed_loop (duplicate of websocket_loop) - handle_frame_output_closed (duplicate of handle_frame) - handle_fragment_output_closed (duplicate of handle_fragment) - common_handle_output_closed (duplicate of common_handle) The unified websocket_loop now handles both normal and output_closed states by checking the output_closed_reason field.
Add pin operator (^) to variables used in size() within bitst 8000 ring pattern matches, as required by newer Elixir versions.
This enables WebSocket connections through HTTP CONNECT proxies, which is
essential for scenarios like:
- Corporate proxy environments
- MITM proxy debugging/inspection
- Network traffic analysis
Changes to Conn module:
- Add :proxy option to connection_option type (accepts URL string or map)
- New open_socket/1 clause for SSL connections with proxy:
1. Connect to proxy via plain TCP
2. Send HTTP CONNECT request to establish tunnel
3. Wait for 200 OK response
4. Upgrade TCP socket to SSL over the tunnel
- Add helper functions:
- parse_proxy_option/1 - handles URL strings and maps
- send_connect_request/2 - sends HTTP CONNECT to proxy
- receive_connect_response/3 - parses proxy response
- parse_connect_headers/2 - consumes response headers
Usage:
WebSockex.start_link(url, module, state,
proxy: "http://localhost:8080"
)
Or with a map:
WebSockex.start_link(url, module, state,
proxy: %{host: "proxy.example.com", port: 8080}
)
|
@cursor rebase this on master resolving the conflicts, and then explain this PR in detail and answer the question whether a) it's still needed, b) makes sense in the current form and c) recommend whether it should be merged yet. |
|
I rebased #141 onto current #141’s head is What the rebase did
On Elixir 1.18.4 / OTP 27, compile with warnings as errors, format, Credo warnings, and Dialyzer pass. The three What this PR actually isWebSockex already has a full close. A callback returning
That matches .NET This is narrower than a TCP half-close of a session that stays open. RFC 6455 says a peer that receives a close frame must answer with its own close as soon as it has finished data it already intends to send. A conforming server will not keep chatting after that frame. The feature matters for peers that deliberately send more data frames after seeing the close, which is what the Relay service does. Does this form make sense?The loop shape does. After review, the duplicate The branch still has defects that make this the wrong revision to land. The proxy commit does not belong here. It is not in the #141 description or in #140, and it has no tests. Half-close itself is wrong in a few places that the tests do not reach:
The tests do not cover the behavior master is missing. They check that RecommendationKeep the feature. Hold the merge until the proxy commit is split out,   |
|
I've split and merged this. Closing as done |
|
Thanks mate |
Summary
This PR adds
close_output/2to support the WebSocket half-close pattern - sending a close frame while continuing to receive incoming frames until the remote side closes.Motivation
The WebSocket protocol supports a "half-close" pattern where one side can signal "I'm done sending" while still receiving data. This is analogous to TCP's
shutdown(SHUT_WR).Use case: Azure Relay's HybridConnection protocol requires this pattern. When forwarding TCP connections through WebSocket-based relay:
shutdown(:write)Without half-close support, the bridge must either:
Changes
WebSockex.close_output/3- Sends a close frame but entersoutput_closed_loopinstead ofclose_loopoutput_closed_loop/4- Processes incoming frames normally, rejects outgoing frames withNotConnectedError{connection_state: :output_closed}:closingand:output_closedstates toNotConnectedErrorUsage
Tests
Added 3 tests covering:
close_outputsend_framereturning appropriate error afterclose_outputCloses #140