-
Notifications
You must be signed in to change notification settings - Fork 24
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
[WIP][RFC] Concept for connection timeout #10
Conversation
A huge 👍 on the feature, but a 👎 on the implementation. As it stands, this implementation doesn't really cancel the connection attempt, it merely discards the resulting connection after the timeout has expired. As such, the event loop keeps waiting for the connection and the socket resource still consumes system resources and network I/O. This feature sounds like a perfect candidate for cancellable promises. However, its spec is still in draft and hence not yet supported in react/promise (see reactphp/promise#14). |
This should be doable now via CancellablePromiseInterface |
@cboden are you planning on updating this PR soon or want me to make a PR to your PR using |
@WyriHaximus Help is always welcome :-) |
@cboden alright will look into it :). |
Added cancellable. Still WIP, API needs adjusting, needs tests. I'm not sure if the trait belongs in this library, maybe Stream instead as it isn't solely tied to a connecting client. |
👍 for pushing this forward, though I'm not sure I like the API and how it's intertwined with the "default" IMO we should first figure out some use cases for this feature to see how this is actually going to be used. The socket-client component is quite lowlevel, so it's unlikely to be used by many users directly. It's probably more likely going to be used as part of some high level abstractions (say, a Redis or MySQL client implementation). These high level components depend on a
I agree that this should probably be moved to a new component instead (promise-timeout or just timeout perhaps?). |
|
||
trait TimeoutTrait | ||
{ | ||
protected function setTimeout(LoopInterface $loop, CancellablePromiseInterface $promise, $seconds = 30) |
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.
Is much gained from having a default value for $seconds
here? Any value is pretty arbitrary.
The implementation for a connection timeout. Any connector could use the method internally to create a connection timeout.