-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
network: add tcp listener backlog config #12625
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ namespace Network { | |
class ListenerImpl : public BaseListenerImpl { | ||
public: | ||
ListenerImpl(Event::DispatcherImpl& dispatcher, SocketSharedPtr socket, ListenerCallbacks& cb, | ||
bool bind_to_port); | ||
bool bind_to_port, uint32_t backlog_size); | ||
void disable() override; | ||
void enable() override; | ||
|
||
|
@@ -25,6 +25,7 @@ class ListenerImpl : public BaseListenerImpl { | |
void setupServerSocket(Event::DispatcherImpl& dispatcher, Socket& socket); | ||
|
||
ListenerCallbacks& cb_; | ||
uint32_t backlog_size_; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. const There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
|
||
private: | ||
void onSocketEvent(short flags); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not have a default param on this interface? I would prefer that we make sure we hit all call sites. How many other places need to change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's definitely possible because the first version of the patch didn't use a default param :-). It's a decent amount of tests, but if you're okay with that, I can push an update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!