mirrors/bun.sh
1
0
Fork 0
mirror of https://github.com/oven-sh/bun synced 2026-02-12 11:59:00 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: correct route registration order for WebSocket handlers

Browse Source 
The key issue was understanding how uWebSockets handles WebSocket routes:

When app.ws() is called, it internally registers a GET handler that:
1. Checks for WebSocket upgrade headers (sec-websocket-key)
2. If present → handles WebSocket upgrade via the upgrade callback
3. If absent → calls req->setYield(true) to pass to next handler

This means app.ws() must be registered BEFORE app.method(.GET) for the
same path, so that:
- WebSocket requests → handled by app.ws()
- Regular GET requests → yield and fall through to GET handler

Changes:
- Register upgrade handler FIRST in user_routes_to_build (before method handlers)
- This ensures correct registration order in server.zig
- Removed websocket_only enum variant (not needed with correct ordering)
- Updated test to expect error on server creation (not connection time)
- Validate that route websocket requires upgrade handler

All 8 tests now passing:
✓ route-specific websocket handlers work independently
✓ route-specific websocket with data in upgrade
✓ route-specific websocket with close handler
✓ global websocket handler still works
✓ mix of route-specific and global websocket handlers
✓ route-specific websocket with multiple HTTP methods
✓ route-specific websocket without upgrade handler errors
✓ server.reload() preserves route-specific websocket handlers

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Claude Bot
2025-10-26 02:56:39 +00:00
parent c25c0ab84e
commit 485017f93f
2 changed files with 45 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
src/bun.js/api/server/ServerConfig.zig
View File

@@ -583,7 +583,6 @@ pub fn fromJS(
var found = false;
var websocket_ctx: ?WebSocketServerContext = null;
var upgrade_callback: jsc.Strong.Optional = .empty;
var has_get_method = false;
// Check for websocket and upgrade fields
if (try value.getOwn(global, "websocket")) |ws_value| {
@@ -598,25 +597,48 @@ pub fn fromJS(
}
}
// Validate: if route has websocket, it must have upgrade
if (websocket_ctx != null and upgrade_callback.impl == null) {
return global.throwInvalidArguments("Route has 'websocket' but missing 'upgrade' handler. Both must be specified together.", .{});
}
// If we have an upgrade handler, add it FIRST (before other method handlers)
// This ensures app.ws() is registered before app.method(.GET), so WebSocket
// upgrade requests are handled by app.ws(), and regular GET requests yield
// and fall through to the GET handler
if (upgrade_callback.impl != null) {
if (!found) {
try validateRouteName(global, path);
}
args.user_routes_to_build.append(.{
.route = .{
.path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)),
.method = .{ .specific = .GET },
},
.callback = upgrade_callback,
.websocket = websocket_ctx, // May be null (uses global)
}) catch |err| bun.handleOom(err);
found = true;
}
// Process HTTP method handlers (registered after upgrade handler)
inline for (methods) |method| {
if (try value.getOwn(global, @tagName(method))) |function| {
if (!found) {
try validateRouteName(global, path);
}
found = true;
if (method == .GET) has_get_method = true;
if (function.isCallable()) {
// Only attach websocket to GET method (WebSocket upgrades use GET)
const ws_for_this_method = if (method == .GET) websocket_ctx else null;
// Never attach websocket to method handlers
// WebSocket is handled separately via the upgrade callback
args.user_routes_to_build.append(.{
.route = .{
.path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)),
.method = .{ .specific = method },
},
.callback = .create(function.withAsyncContextIfNeeded(global), global),
.websocket = ws_for_this_method,
.websocket = null,
}) catch |err| bun.handleOom(err);
} else if (try AnyRoute.fromJS(global, path, function, init_ctx)) |html_route| {
var method_set = bun.http.Method.Set.initEmpty();
@@ -631,33 +653,6 @@ pub fn fromJS(
}
}
// If we have websocket/upgrade but no GET handler, create one
// WebSocket upgrades require GET method
if (found and !has_get_method and (websocket_ctx != null or upgrade_callback.impl != null)) {
args.user_routes_to_build.append(.{
.route = .{
.path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)),
.method = .{ .specific = .GET },
},
.callback = upgrade_callback,
.websocket = websocket_ctx,
}) catch |err| bun.handleOom(err);
}
// If we have websocket/upgrade but no method handlers at all, create a route for them
if (!found and (websocket_ctx != null or upgrade_callback.impl != null)) {
try validateRouteName(global, path);
args.user_routes_to_build.append(.{
.route = .{
.path = bun.handleOom(bun.default_allocator.dupeZ(u8, path)),
.method = .any,
},
.callback = upgrade_callback,
.websocket = websocket_ctx,
}) catch |err| bun.handleOom(err);
found = true;
}
if (found) {
bun.default_allocator.free(path);
continue;

41
test/js/bun/http/serve-route-websocket.test.ts
View File

@@ -268,33 +268,26 @@ describe("Bun.serve() route-specific WebSocket handlers", () => {
ws.close();
});
test("route-specific websocket without upgrade handler errors appropriately", async () => {
using server = Bun.serve({
port: 0,
routes: {
"/ws": {
websocket: {
open(ws) {
ws.send("should not reach here");
test("route-specific websocket without upgrade handler errors appropriately", () => {
// Should throw an error because websocket requires upgrade handler
expect(() => {
Bun.serve({
port: 0,
routes: {
"/ws": {
websocket: {
open(ws) {
ws.send("should not reach here");
},
},
// Note: no upgrade handler
GET() {
return new Response("This is not a WebSocket endpoint");
},
},
// Note: no upgrade handler
GET() {
return new Response("This is not a WebSocket endpoint");
},
},
},
});
// This should fail to upgrade since there's no upgrade() handler
const ws = new WebSocket(`ws://localhost:${server.port}/ws`);
let errorOccurred = false;
ws.onerror = () => {
errorOccurred = true;
};
await new Promise(resolve => setTimeout(resolve, 500));
expect(errorOccurred).toBe(true);
});
}).toThrow("Route has 'websocket' but missing 'upgrade' handler");
});
test("server.reload() preserves route-specific websocket handlers", async () => {