-
Notifications
You must be signed in to change notification settings - Fork 1.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
Update amp #5170
Update amp #5170
Conversation
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.
LGTM
frameworks/PHP/amp/server.php
Outdated
|
||
Amp\Loop::run(function () { | ||
$sockets = yield [ | ||
Cluster::listen("0.0.0.0:8080"), | ||
Cluster::listen("[::]:8080"), | ||
Cluster::listen('0.0.0.0:8080') |
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.
This should probably pass a customized BindContext
and increase the default backlog size.
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, 128 is very low for a server.
But I don't know enough Amp.
Please could you help with that?
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.
Sure. All you need to do is create an instance of (new Amp\Socket\BindContext)->withBacklog($size)
as second parameter.
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.
Added socket bind context with backlog(102400)
as the stream_socket_server
use 2 sockets.
And with reuseport
and tcpnodelay
.
But I miss an option to set keepalive
in the BindContext class
.
Where could we set the keepalive
??
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.
@joanhey I stumbled upon that yesterday, too, so I just opened amphp/socket#67.
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.
Without It's ready in the amp/http/serverkeepalive
, never will be fast, as each request from the same client need to open a new tcp connection.
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.
Oh, we're talking about different keep alives. HTTP/1.1 keep alive / connection reuse is automatically enabled.
The thing I just opened is about Tcp keep alive and only affects idle connections.
Bigger backlog, reuseport and tcpnodelay
* Update amp * Update connection limit * Add socket bind context Bigger backlog, reuseport and tcpnodelay
Update:
Ping @kelunik to review