Disable idle connection kickoff in drogon - #362
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (6)
🔇 Additional comments (1)
WalkthroughThe Drogon HTTP server startup now sets the idle connection timeout to Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp`:
- Line 473: Update the Drogon server configuration around
setIdleConnectionTimeout so it no longer passes 0 globally. Set a bounded idle
timeout aligned with the maximum calculation duration, or implement
request-specific handling that preserves idle connection cleanup for unrelated
connections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0c48468e-8cd4-49c5-abde-8ec16365820a
📒 Files selected for processing (1)
connection_scan_algorithm/src/transit_routing_http_server.cpp
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Test build on ubuntu-latest (memcached: false)
- GitHub Check: Test build on macOS-latest (memcached: false)
- GitHub Check: Analyze (cpp)
- GitHub Check: Test build on macOS-latest (memcached: true)
- GitHub Check: Test build on ubuntu-latest (memcached: true)
- GitHub Check: pr-build-check
| drogon::app() | ||
| .addListener("0.0.0.0", programOptions.port) | ||
| .setThreadNum(4) // IO event loops only; calculations run on the compute pool | ||
| .setIdleConnectionTimeout(0) // 0 disables the kickoff of idle connection, we want to wait for long calculation |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'transit_routing_http_server\.cpp|CMakeLists\.txt|drogon.*|package.*|requirements|Dockerfile|docker-compose|README|.*config.*' . | sed 's#^\./##' | head -200
echo
echo "== relevant snippet =="
if [ -f connection_scan_algorithm/src/transit_routing_http_server.cpp ]; then
nl -ba connection_scan_algorithm/src/transit_routing_http_server.cpp | sed -n '440,500p'
fi
echo
echo "== drogon idle timeout/config references =="
rg -n "setIdleConnectionTimeout|idleConnectionTimeout|idle.*timeout|keepAlive|keep_alive|keepAliveTimeout|keepAlive" . -S --glob '!build/**' --glob '!node_modules/**' --glob '!dist/**' | head -300Repository: chairemobilite/trRouting
Length of output: 1144
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git changed files/stat =="
git status --short || true
git diff --stat || true
echo
echo "== inspect connection server around dispatch if present =="
if [ -f connection_scan_algorithm/src/transit_routing_http_server.cpp ]; then
wc -l connection_scan_algorithm/src/transit_routing_http_server.cpp
nl -ba connection_scan_algorithm/src/transit_routing_http_server.cpp | sed -n '1,180p'
fiRepository: chairemobilite/trRouting
Length of output: 358
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== relevant snippet =="
awk '{printf "%6d\t%s\n", NR, $0}' connection_scan_algorithm/src/transit_routing_http_server.cpp | sed -n '430,490p'
echo
echo "== keepalive/idle timeout references =="
grep -RInE "setIdleConnectionTimeout|idleConnectionTimeout|idle.*timeout|keepAlive|keep_alive|keepAliveTimeout|keepAlive" . --exclude-dir=build --exclude-dir=result --exclude-dir=node_modules --exclude-dir=dist 2>/dev/null | head -300
echo
echo "== drogon version hints =="
grep -RInEi "drogon|libdrogon|drogon|drogon.*2\.[0-9]|0\.[0-9]+\.[0-9]+" . -E --exclude-dir=build --exclude-dir=result --exclude-dir=node_modules --exclude-dir=dist 2>/dev/null | head -200Repository: chairemobilite/trRouting
Length of output: 8619
🌐 Web query:
Drogon HttpServer idleConnectionTimeout 0 infinite keeps idle connections
💡 Result:
Yes, in the Drogon framework, setting the idleConnectionTimeout to 0 effectively means that idle connections will not be closed by the server [1][2]. According to the official Drogon documentation and header files, the idleConnectionTimeout parameter defines the lifetime of a connection without any read or write activity [1][2]. While the default value is 60 seconds, setting this timeout to 0 instructs Drogon not to close idle connections [1][2]. This configuration can be applied either programmatically via the HttpAppFramework interface or through the framework's configuration file [1][2][3].
Citations:
- 1: https://github.com/drogonframework/drogon/blob/59cd4366/lib/inc/drogon/HttpAppFramework.h
- 2: https://github.com/drogonframework/drogon/blob/master/lib/inc/drogon/HttpAppFramework.h
- 3: https://github.com/drogonframework/drogon/blob/master/config.example.yaml
🌐 Web query:
Drogon documentation connection idle timeout no reads writes idleConnectionTimeout=0 infinite
💡 Result:
In the Drogon framework, setting the idleConnectionTimeout (or idle_connection_timeout in the configuration file) to 0 disables the idle connection timeout mechanism [1][2][3]. This effectively makes connections "infinite" in terms of idle duration, meaning Drogon will not forcibly close connections due to inactivity (i.e., when there are no reads or writes) [1][2][4][5]. By default, this timeout is set to 60 seconds [4][6][7]. When a connection remains idle (no read or write activity) for longer than the specified timeout value, Drogon will close the connection [4][5]. You can configure this behavior in two primary ways: 1. Through the configuration file (JSON/YAML): Set the idle_connection_timeout key to 0 [4][6]. "idle_connection_timeout": 0 2. Programmatically using the app() interface: Use the setIdleConnectionTimeout method [1][8]. app.setIdleConnectionTimeout(0); This setting applies to the lifetime of connections managed by the framework, helping prevent the server from terminating connections that may otherwise be waiting for data [1][4][9].
Citations:
- 1: https://github.com/drogonframework/drogon/blob/master/lib/inc/drogon/HttpAppFramework.h
- 2: https://github.com/drogonframework/drogon/blob/acad9c8e/lib/inc/drogon/HttpAppFramework.h
- 3: https://github.com/drogonframework/drogon/blob/59cd4366/lib/inc/drogon/HttpAppFramework.h
- 4: https://github.com/drogonframework/drogon/wiki/ENG-11-Configuration-File
- 5: https://codeberg.org/r7n/drogon/wiki/ENG-10-Configuration-File.md
- 6: https://github.com/drogonframework/drogon/blob/master/config.example.yaml
- 7: https://github.com/drogonframework/drogon/blob/e8905597/drogon_ctl/templates/config_yaml.csp
- 8: Websocket connection closes with code 1006 drogonframework/drogon#124
- 9: Websocket connection closes with code 1006 drogonframework/drogon#124
Avoid disabling idle timeouts globally.
0 removes the idle timeout for all connections, not just long-running calculation requests. Since Drogon treats idle connections as ones with no reads or writes, clients that stop sending data after start can remain open indefinitely and exhaust sockets/memory under load. Use a bounded timeout aligned with the maximum calculation duration, or handle long-running requests without disabling idle cleanup globally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@connection_scan_algorithm/src/transit_routing_http_server.cpp` at line 473,
Update the Drogon server configuration around setIdleConnectionTimeout so it no
longer passes 0 globally. Set a bounded idle timeout aligned with the maximum
calculation duration, or implement request-specific handling that preserves idle
connection cleanup for unrelated connections.
|
I would put a very big value so stuck connection do timeout eventually |
So what is the really big max of a calculation you can imagine for trRouting to do ? |
|
I don't know. There is no way to check if trrouting is indeed calculating or just stuck? |
By default, drogon would kill a connection that is idle for 60 seconds. It's quite common for calculation to take more time than that, so we don't want this behavior. We set it to a 20 minutes timeout, to still clean really idle connection. We could consider in the future to send data on the pipe to keep the connection alive, or implement a response polling mecanism
0a0e9b1 to
620b5fb
Compare
CPU usage and timing clearly shows that trrouting was calculating |
2 participants
By default, drogon would kill a connection that is idle for 60 seconds. It's quite common for calculation to take more time than that, so we don't want this behavior.
We could consider in the future to set it to a really high value.
Summary by CodeRabbit