net: add set_listen_backlog() to update backlog on a live server socket#3312
net: add set_listen_backlog() to update backlog on a live server socket#3312ScyllaPiotr wants to merge 1 commit into
Conversation
| virtual future<accept_result> accept() override; | ||
| virtual void abort_accept() override; | ||
| virtual socket_address local_address() const override; | ||
| virtual void set_listen_backlog(int backlog) override { _lfd.get_file_desc().listen(backlog); } |
There was a problem hiding this comment.
Shouldn't TLS socket also get its set_listen_backlog() override?
There was a problem hiding this comment.
Good catch, added the forwarding override in server_session so TLS sockets delegate to the underlying socket.
ScyllaPiotr
force-pushed
the
fix-ent-5614-seastar
branch
from
e33e081 to
4fb87e3
Compare
| virtual future<accept_result> accept() override; | ||
| virtual void abort_accept() override; | ||
| virtual socket_address local_address() const override; | ||
| virtual void set_listen_backlog(int backlog) override { _lfd.get_file_desc().listen(backlog); } |
There was a problem hiding this comment.
Please move the implementation to .cc to reduce include load.
There was a problem hiding this comment.
Done. Moved both posix_server_socket_impl::set_listen_backlog and posix_reuseport_server_socket_impl::set_listen_backlog implementations to posix-stack.cc, consistent with the other methods in those classes.
| /// The default implementation is a no-op; POSIX implementations | ||
| /// re-invoke the listen() syscall which Linux supports on a | ||
| /// bound socket. | ||
| virtual void set_listen_backlog(int backlog) {} |
There was a problem hiding this comment.
Changed the default to log a warning instead of silently no-op'ing, following the pattern in native-stack-impl.hh where unsupported operations (e.g. Reuseaddr is not supported by native stack) are logged. The implementation is now in stack.cc, keeping the header clean.
ScyllaPiotr
force-pushed
the
fix-ent-5614-seastar
branch
from
4fb87e3 to
2754899
Compare
|
@ScyllaPiotr there are merge conflicts. |
|
By the way, this is a single-patch commit so its cover letter gets ignored and isn't saved in the git log. If there is any important information in the cover letter you wish to save, please copy to the commit message of the single patch. Thanks. |
nyh
left a comment
nyh
left a comment
There was a problem hiding this comment.
Looks good to me, but you are missing a test.
If you tested this externally (e.g., in ScyllaDB) it will be good to say so, but even better would be to add a unit test. Should be fairly easy - create a server with opts.listen_backlog = 42, confirm the listen_backlog is 42, then set_listen_backlog(67) and check that listen_backlog changed to 67.
By the way, do we actually have a function get_server_backlog() to use in that test? If we don't, this test will help you realize that we're missing this ;-)
| virtual void abort_accept() = 0; | ||
| virtual socket_address local_address() const = 0; | ||
| /// Update the listen backlog on an already-listening socket. | ||
| /// The default implementation logs a warning; POSIX implementations |
There was a problem hiding this comment.
Nitpick: I don't think the superclass should document how the subclasses handle this API. The word "default implementation" is even misleading - the default socket is the Posix one, the superclass is not really "default", it's just a superclass that nobody should be using directly.
Add set_listen_backlog(int) to the server_socket API and expose the same operation through http_server so active listeners can be updated after listen(). Add get_listen_backlog() for observability/testing, implement it in POSIX server socket implementations by tracking the configured backlog, and forward it through TLS server sockets. On POSIX sockets, set_listen_backlog() re-invokes listen(backlog) on the listening file descriptor. Add a unit test that verifies runtime update of the configured backlog value (1111 -> 1888).
ScyllaPiotr
force-pushed
the
fix-ent-5614-seastar
branch
from
2754899 to
09606fe
Compare
|
Addressed in 09606fe:
Local verification: @nyh please take another look when convenient. |
|
@avikivity please re-review |
@nyh please re-review. |
4 participants
Summary
set_listen_backlog(int)to theserver_socketclass hierarchy, allowing the listen backlog to be changed on an already-listening socketlisten()syscall on the underlying file descriptor, which is supported for bound socketsset_listen_backlog()tohttp_server, which iterates all active listeners and updates their backlogDetails
The virtual method is added to
server_socket_implwith a no-op default. The POSIX implementations (posix_server_socket_implandposix_reuseport_server_socket_impl) override it to call_lfd.get_file_desc().listen(backlog). The public API is exposed throughserver_socket::set_listen_backlog()andhttp_server::set_listen_backlog().This is needed by ScyllaDB's Alternator to support live-updating the listen socket backlog configuration at runtime.