rusk: implement RuskHttp builder and shutdown#4006
rusk: implement RuskHttp builder and shutdown#4006Neotamandua wants to merge 1 commit intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a builder pattern for RuskHttp with proper shutdown handling, addressing test panic messages. The changes restructure the HTTP server to support graceful shutdown with configurable timeouts and eliminate a redundant runtime.
Changes:
- Added
RuskHttpstruct withrun()andshutdown()methods to manage HTTP server lifecycle - Modified
HttpServerto support graceful shutdown by makingwait()consume handle once and addingshutdown()method - Removed nested tokio runtime in
listening_loopthat was causing shutdown issues
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| rusk/src/lib/http.rs | Changed HttpServer fields to Option types and added shutdown() method; removed nested runtime from listening_loop |
| rusk/src/lib/builder/http_only.rs | Added RuskHttp struct with lifecycle methods and enhanced builder with data source and timeout configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
e06c296 to
022b975
Compare
022b975 to
b4efde3
Compare
b4efde3 to
0a16c5c
Compare
There was a problem hiding this comment.
I would approve this PR, but the removal of the dedicated HTTP runtime in listening_loop slightly concerns me.
HTTP connections now share the main runtime via task::spawn, resulting in heavy HTTP/RUES traffic potentially starving consensus. We should keep separate runtimes for chain mode or add concurrency limiting in my opinion. Consensus/core chain should always have priority over anything else.
What do you think @herr-seppia?
Closes #2453
Fixes the panic messages in tests when running:
cargo test --release --lib http::tests -- --nocapture