Conversation
This commit introduces the `IsWebSocket` flag to `RouteConfigBase`.
When set to true, this flag ensures that the router's global or sub-router
timeout configurations are ignored for the specific route, effectively
disabling the timeout middleware. This is necessary for long-lived
WebSocket connections that would otherwise be terminated by the timeout.
Changes:
- Modified `RouteConfigBase` in `pkg/router/config.go` to include `IsWebSocket`.
- Updated `registerSubRouter` in `pkg/router/router.go` to override timeout to 0 if `IsWebSocket` is true.
- Updated `RegisterRoute` in `pkg/router/route.go` to override timeout to 0 if `IsWebSocket` is true.
- Added `TestWebSocketRoute` in `pkg/router/websocket_test.go`.
- Updated `README.md` with documentation and usage examples.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
+ Coverage 97.06% 97.28% +0.21%
==========================================
Files 18 18
Lines 2382 1802 -580
==========================================
- Hits 2312 1753 -559
+ Misses 57 37 -20
+ Partials 13 12 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1,73 @@ | |||
| package router_test | |||
There was a problem hiding this comment.
Test coverage for the patch is low. Actually around 50%. Please add tests so that we will have test coverage of the entire patch.
There was a problem hiding this comment.
Added comprehensive tests for WebSocket routes, including support within sub-routers.
Expanded the test suite to cover WebSocket support in both top-level
routers and sub-routers. This ensures that the `IsWebSocket` flag
correctly disables timeouts in all routing scenarios.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
pkg/router/router.go
Outdated
There was a problem hiding this comment.
Even though IsWebSocket is meant to enable WebSocket routes by skipping the timeout here, the handler still receives metricsResponseWriter whenever trace logging is enabled (TraceIDBufferSize > 0 in ServeHTTP). That wrapper (router.go ~814) does not implement http.Hijacker, so a WebSocket upgrade like the README example (upgrader.Upgrade) will fail with response does not implement http.Hijacker unless tracing is disabled. The new flag therefore doesn’t actually permit WebSocket upgrades under a common configuration; the wrapper needs to preserve Hijack or be skipped for IsWebSocket routes.
Useful? React with 👍 / 👎.
* feat: add minimal websocket example app - Add `github.com/gorilla/websocket` dependency. - Create `examples/websocket/main.go` to demonstrate functional WebSocket support. - Include a client test in the example to verify REST and WebSocket endpoints. - Prove that `IsWebSocket: true` bypasses global timeouts. * refactor: simplify router initialization and update dependencies --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Suhaib <suhaib.abdulquddos@gmail.com>
* feat: add minimal websocket example app - Add `github.com/gorilla/websocket` dependency. - Create `examples/websocket/main.go` to demonstrate functional WebSocket support. - Include a client test in the example to verify REST and WebSocket endpoints. - Prove that `IsWebSocket: true` bypasses global timeouts. * refactor: simplify router initialization and update dependencies * fix: improve timeout handling in mutexResponseWriter to prevent race conditions --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
This is a breaking change that renames the `IsWebSocket` field to `DisableTimeout` in `RouteConfigBase`. This new name better reflects the flag's purpose, which is to disable the global or sub-router timeout for any long-lived connection, such as WebSockets or Server-Sent Events (SSE). Updates: - `pkg/router/config.go`: Renamed field and updated comments. - `pkg/router/route.go` & `pkg/router/router.go`: Updated logic to use `DisableTimeout`. - `pkg/router/websocket_test.go`: Updated tests. - `examples/websocket/main.go`: Updated example. - `README.md`: Updated documentation. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
1 participant
Implemented WebSocket support by allowing routes to opt-out of global timeouts via a new
IsWebSocketflag inRouteConfigBase.PR created automatically by Jules for task 16871215110249131027 started by @Suhaibinator