Conversation
74451e5 to
e28a5b0
Compare
There was a problem hiding this comment.
Pull request overview
Adds WebSocket (libwebsockets) support to the embedded broker, aligns the WebSocket client example with the libwebsockets TLS model, and updates docs/tests/CI to exercise the new transport.
Changes:
- Add a libwebsockets-backed WebSocket listener to the broker and route broker-side
MqttNetI/O through WebSocket callbacks. - Adjust TLS socket helper compilation guards and update the WebSocket client example to avoid wolfSSL socket-TLS setup (TLS handled by libwebsockets).
- Extend broker tests and CI to build/run with WebSocket enabled; update
AGENTS.md.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfmqtt/mqtt_socket.h | Broaden TLS helper function visibility when WebSocket is enabled. |
| wolfmqtt/mqtt_broker.h | Add broker and per-client fields for WebSocket support. |
| src/mqtt_socket.c | Match TLS helper compilation guards with header changes. |
| src/mqtt_broker.c | Implement broker-side libwebsockets server + integration into broker loop/CLI. |
| src/include.am | Link broker binary with -lwebsockets when WebSocket is built. |
| scripts/broker.test | Add an end-to-end WebSocket connect/subscribe test. |
| examples/websocket/websocket_client.c | Prevent socket-TLS setup for WebSocket connections (TLS handled by LWS). |
| examples/websocket/net_libwebsockets.c | Add compatibility macro for protocol list terminator. |
| AGENTS.md | Document broker feature and testing/build options. |
| .github/workflows/broker-check.yml | Add CI matrix entries for broker builds with WebSocket (and WebSocket+TLS). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Add server-side WebSocket support via libwebsockets, allowing MQTT-over-WebSocket clients to connect alongside regular TCP clients. The broker listens on two ports simultaneously (TCP and WebSocket) with per-client rx/tx buffers bridging the lws push-based callback model to the broker's pull-based polling model. - Add BrokerWsCtx per-client context and WebSocket fields to MqttBroker - Add lws server callback, WS-specific MqttNet read/write/disconnect - Add BrokerClient_AddWs for WebSocket client initialization - Add -w <port> CLI option to enable WebSocket listening - Add local TLS IO callbacks when both WebSocket and TLS are enabled - Fix wolfSSL/OpenSSL header conflict in net_libwebsockets.c client - Link libwebsockets for broker when BUILD_WEBSOCKET is enabled - Add WebSocket test case to broker.test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add two new matrix entries to broker-check.yml for testing the broker with WebSocket support: plain WebSocket and WebSocket + TLS. Installs libwebsockets-dev and configures wolfSSL with --enable-opensslcoexist for system lws compatibility. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b1f502d to
dba0ca8
Compare
| else | ||
| #endif | ||
| { | ||
| (void)BrokerNetDisconnect(bc); |
There was a problem hiding this comment.
Consider removing this call, as its done already below at line 950. Or visa versa
| if (bc_ptr != NULL) { | ||
| *bc_ptr = NULL; | ||
| } | ||
| lws_close_reason(ws->wsi, LWS_CLOSE_STATUS_NORMAL, NULL, 0); |
There was a problem hiding this comment.
lws_close_reason() sets a close code for use when you return -1 from inside a callback. Called from BrokerWsNetDisconnect (outside any callback), it has no effect — the WebSocket close handshake frame is never sent. The remote peer won't receive a proper close frame. A lws_callback_on_writable + return -1 pattern is needed, or lws_wsi_close() (available in newer lws).
| /* Request writable callback and service until data is flushed */ | ||
| lws_callback_on_writable((struct lws*)ws->wsi); | ||
| while (ws->tx_pending != NULL && ws->status > 0 && attempts < 100) { | ||
| lws_service(lws_get_context((struct lws*)ws->wsi), 0); |
There was a problem hiding this comment.
This works in practice (single-threaded, and the test passes), but it's architecturally wrong for lws — lws_service re-enters the event loop, processing callbacks for all connections while waiting for one write. If the write doesn't flush in 100 iterations it returns a network error. It happens to work now because the test scenario is simple, but under load or on a slow link this could spin or fail.
| if (ws->tx_pending != NULL && ws->tx_len > 0) { | ||
| int n = lws_write(wsi, ws->tx_pending + LWS_PRE, | ||
| ws->tx_len, LWS_WRITE_BINARY); | ||
| if (n < (int)ws->tx_len) { |
There was a problem hiding this comment.
Per lws 4.x docs, lws_write returns -1 on failure or the number of bytes written on success. For binary frames it's expected to write all bytes or return -1. The n < tx_len check would also catch n == 0, treating it as a failure (correct). This is actually fine but a comment explaining the lws guarantee would help.
| #endif | ||
| } | ||
|
|
||
| #ifdef WOLFMQTT_BROKER_WILL |
There was a problem hiding this comment.
This guard is still needed.
| #endif | ||
| #endif | ||
| #ifdef ENABLE_MQTT_WEBSOCKET | ||
| void *ws_ctx; /* struct lws_context* (opaque) */ |
There was a problem hiding this comment.
Consider a different name here confusing
ws_ctx field name used in both MqttBroker (for lws_context*) and BrokerClient (for BrokerWsCtx*)
Add websocket support to broker.
Also cleanup client lib websocket logic.
Also update agents.md
Added broker with websockets tests