Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions packages/bun-uws/src/App.h
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,25 @@ struct TemplatedApp {
return std::move(*this);
}

/* Send a close frame to every open WebSocket and fire its close handler.
* Unlike close(), this performs the WebSocket closing handshake so peers
* observe the given status (e.g. 1001 Going Away) rather than 1006. */
TemplatedApp &&endAllWebSockets(int code, std::string_view message = {}) {
for (us_socket_group_t *g : webSocketGroups) {
struct us_socket_t *s = g->head_sockets;
while (s) {
struct us_socket_t *next = s->next;
if (!us_socket_is_closed(s)) {
/* USERDATA is erased in the handler slots; see the
* TopicTree cast above. end() no-ops on isShuttingDown. */
((WebSocket<SSL, true, int> *) s)->end(code, message);
}
s = next;
}
}
return std::move(*this);
Comment thread
robobun marked this conversation as resolved.
}

/** Closes all connections connected to this server which are not sending a request or waiting for a response. Does not close the listen socket. */
TemplatedApp &&closeIdle() {
auto *group = httpContext->getSocketGroup();
Expand Down
91 changes: 62 additions & 29 deletions src/runtime/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1628,22 +1628,22 @@
}
}

let Some(listener) = self.listener.take() else {
let listener = self.listener.take();
if listener.is_none() {
if Self::HAS_H3 && self.h3_app.is_some() {
self.unref();
self.notify_inspector_server_stopped();
if abrupt {
self.flags.insert(ServerFlags::TERMINATED);
}
}
// A prior graceful stop already took the listener. An abrupt stop
// still needs to tear down in-flight connections so a "graceful
// then force" shutdown can complete.
if abrupt {
self.end_all_websockets_going_away();
self.terminate_app();
}
return;
};
if abrupt || (self.pending_requests == 0 && !self.has_active_web_sockets()) {
self.unref();
}
// A graceful stop with work in flight keeps the ref (deinit_if_we_can
// unrefs when the drain completes): on Windows uv_run skips I/O with
// zero ref'd handles, so unrefing here wedged server.close() teardown.
let listener = listener.unwrap();

Check warning on line 1646 in src/runtime/server/mod.rs

View check run for this annotation

Claude / Claude Code Review

let-else rewritten to is_none() + unwrap() anti-pattern

nit: this rewrites the original `let Some(listener) = self.listener.take() else { ...; return; };` into `is_none()` + `return` + `.unwrap()`, which REVIEW.md → *Code style & idioms* explicitly lists as an anti-pattern ("`if let`/`?` over null-check-then-unwrap"). There's no borrow-checker constraint forcing it — the else arm of a let-else binds nothing, so `self.end_all_websockets_going_away()` / `self.terminate_app()` compile fine inside it (the pre-PR else block already called `self.unref()` t
Comment thread
robobun marked this conversation as resolved.
Outdated

if !SSL {
// SAFETY: `listener` is a live uws ListenSocket FFI handle just taken
Expand All @@ -1664,31 +1664,64 @@
}
}

// Send 1001 Going Away to every open WebSocket so peers see a clean
// protocol close instead of a dropped connection (1006), and so the
// graceful-stop promise can observe the count reaching zero.
self.end_all_websockets_going_away();

Check failure on line 1670 in src/runtime/server/mod.rs

View check run for this annotation

Claude / Claude Code Review

node:http Server.close() now force-closes WebSocket connections

This regresses `node:http` `Server#close()`: `_http_server.ts:514-515` calls `server.stop()` (graceful), and every node:http server unconditionally passes a `websocket:` block whose sockets the `ws`-package shim populates via `server.upgrade()` — so after this change `http.Server#close()` sends 1001 to every open `ws` connection. In Node.js `server.close()` only stops accepting new connections and leaves upgraded sockets to the user, so the standard `httpServer.close()` + `wss.clients.forEach(c
Comment thread
robobun marked this conversation as resolved.

if abrupt || (self.pending_requests == 0 && !self.has_active_web_sockets()) {
self.unref();
}
// A graceful stop with work in flight keeps the ref (deinit_if_we_can
// unrefs when the drain completes): on Windows uv_run skips I/O with
// zero ref'd handles, so unrefing here wedged server.close() teardown.

if !abrupt {
// S012: `app::ListenSocket<SSL>` is a ZST opaque — safe deref.
bun_opaque::opaque_deref_mut(listener).close();
} else if !self.flags.contains(ServerFlags::TERMINATED) {
if let Some(ws) = self.config.websocket.as_mut() {
ws.handler.app = None;
}
self.flags.insert(ServerFlags::TERMINATED);
// `app.close()` synchronously drains every open websocket; their
// `on_close` defers call `on_websocket_closed`, which would
// dispatch `deinit_if_we_can` through a fresh `&mut NewServer`
// while this frame still holds `&mut self`. Hold the re-entrance
// guard across the drain so that nested call early-returns —
// `stop()` runs `deinit_if_we_can` itself right after this returns.
} else {
self.terminate_app();
}
}

fn end_all_websockets_going_away(&mut self) {
if !self.has_active_web_sockets() {
return;
}
let Some(app) = self.app else { return };
// `end()` fires the close handler synchronously; its `on_close` defer
// calls `on_websocket_closed`, which would dispatch `deinit_if_we_can`
// through a fresh `&mut NewServer` while this frame still holds
// `&mut self`. Hold the re-entrance guard across the drain so the
// nested call early-returns; `stop()` runs the idle pass afterwards.
self.deinit_running.set(true);
// S012: `NewApp<SSL>` is a ZST opaque — safe `*mut → &mut` deref.
bun_opaque::opaque_deref_mut(app).end_all_websockets(1001, b"Server closed");
self.deinit_running.set(false);
}
Comment thread
robobun marked this conversation as resolved.

/// Force-close every connection on the uws app and mark the server
/// terminated. Guarded by `TERMINATED` so repeated abrupt stops are no-ops.
fn terminate_app(&mut self) {
if self.flags.contains(ServerFlags::TERMINATED) {
return;
}
if let Some(ws) = self.config.websocket.as_mut() {
ws.handler.app = None;
}
self.flags.insert(ServerFlags::TERMINATED);
if let Some(app) = self.app {
self.deinit_running.set(true);
// S012: `NewApp<SSL>` is a ZST opaque — safe `*mut → &mut` deref.
bun_opaque::opaque_deref_mut(self.app.unwrap()).close();
bun_opaque::opaque_deref_mut(app).close();
self.deinit_running.set(false);
// Only clear after the drain — `on_close` defers reach
// `on_websocket_closed` through `handler.server`, so wiping it
// earlier would strand the live-socket count and the idle pass
// would never see it drained.
if let Some(ws) = self.config.websocket.as_mut() {
ws.handler.server = None;
}
}
// Only clear after the drain — `on_close` defers reach
// `on_websocket_closed` through `handler.server`, so wiping it
// earlier would strand the live-socket count and the idle pass
// would never see it drained.
if let Some(ws) = self.config.websocket.as_mut() {
ws.handler.server = None;
}
}

Expand Down
20 changes: 10 additions & 10 deletions src/runtime/server/server_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2549,23 +2549,23 @@ where
pub fn stop_from_js(&mut self, abruptly: Option<JSValue>) -> JSValue {
let rc = self.get_all_closed_promise(&self.global());

if self.has_listener() {
let abrupt = 'brk: {
if let Some(val) = abruptly {
if val.is_boolean() && val.to_boolean() {
break 'brk true;
}
let abrupt = 'brk: {
if let Some(val) = abruptly {
if val.is_boolean() && val.to_boolean() {
break 'brk true;
}
false
};
}
false
};
if self.has_listener() || (abrupt && !self.flags.contains(ServerFlags::TERMINATED)) {
self.stop(abrupt);
}

rc
}

pub fn dispose_from_js(&mut self) -> JSValue {
if self.has_listener() {
if self.has_listener() || !self.flags.contains(ServerFlags::TERMINATED) {
self.stop(true);
}
JSValue::UNDEFINED
Expand Down Expand Up @@ -2742,7 +2742,7 @@ where
}

pub fn get_all_closed_promise(&mut self, global: &JSGlobalObject) -> JSValue {
if !self.has_listener() && self.pending_requests == 0 {
if !self.has_listener() && self.pending_requests == 0 && !self.has_active_web_sockets() {
return JSPromise::resolved_promise(global, JSValue::UNDEFINED).to_js();
}
if self.all_closed_promise.has_value() {
Expand Down
21 changes: 21 additions & 0 deletions src/uws_sys/App.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,20 @@ impl<const SSL: bool> App<SSL> {
c::uws_app_close_idle(Self::SSL_FLAG, self.as_raw())
}

pub fn end_all_websockets(&mut self, code: i32, message: &[u8]) {
// SAFETY: `message` is only read for the synchronous duration of the
// call; uWS copies it into the close frame before returning.
unsafe {
c::uws_app_end_all_websockets(
Self::SSL_FLAG,
self.as_raw(),
code,
message.as_ptr(),
message.len(),
)
}
}

pub fn create(opts: &BunSocketContextOptions) -> Option<*mut Self> {
// SAFETY: FFI call; uws_create_app returns null on failure.
let app = unsafe { c::uws_create_app(Self::SSL_FLAG, *opts) };
Expand Down Expand Up @@ -513,6 +527,13 @@ pub mod c {
unsafe extern "C" {
pub(crate) safe fn uws_app_close(ssl: i32, app: &mut uws_app_s);
pub(crate) safe fn uws_app_close_idle(ssl: i32, app: &mut uws_app_s);
pub(crate) fn uws_app_end_all_websockets(
ssl: i32,
app: &mut uws_app_s,
code: c_int,
message: *const u8,
length: usize,
);
// safe: `&mut uws_app_s` is ABI-identical to a non-null `*mut`;
// `handler`/`user_data` are stored opaquely (never dereferenced by the
// C++ shim itself) — no preconditions on this call.
Expand Down
15 changes: 15 additions & 0 deletions src/uws_sys/libuwsockets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,21 @@ extern "C"
}
}

void uws_app_end_all_websockets(int ssl, uws_app_t *app, int code, const char *message, size_t length)
{
std::string_view msg = message ? std::string_view(message, length) : std::string_view();
if (ssl)
{
uWS::SSLApp *uwsApp = (uWS::SSLApp *)app;
uwsApp->endAllWebSockets(code, msg);
}
else
{
uWS::App *uwsApp = (uWS::App *)app;
uwsApp->endAllWebSockets(code, msg);
}
}

void uws_app_set_on_clienterror(int ssl, uws_app_t *app, void (*handler)(void *user_data, int is_ssl, struct us_socket_t *rawSocket, uint8_t errorCode, char *rawPacket, int rawPacketLength), void *user_data)
{
if (ssl)
Expand Down
Loading
Loading