Skip to content
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

Closed
wants to merge 3 commits into from
Closed

[WIP][RFC] Concept for connection timeout #10

wants to merge 3 commits into from

Conversation

cboden
Copy link
Contributor

@cboden cboden commented Jun 15, 2014

The implementation for a connection timeout. Any connector could use the method internally to create a connection timeout.

@cboden cboden added the feature label Jun 15, 2014
@clue
Copy link
Contributor

clue commented Aug 17, 2014

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).

@cboden
Copy link
Contributor Author

cboden commented Dec 9, 2014

This should be doable now via CancellablePromiseInterface

@WyriHaximus
Copy link
Contributor

@cboden are you planning on updating this PR soon or want me to make a PR to your PR using CancellablePromiseInterface?

@cboden
Copy link
Contributor Author

cboden commented Feb 13, 2015

@WyriHaximus Help is always welcome :-)

@WyriHaximus
Copy link
Contributor

@cboden alright will look into it :).

@cboden
Copy link
Contributor Author

cboden commented May 16, 2015

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.

@clue
Copy link
Contributor

clue commented Jun 6, 2015

👍 for pushing this forward, though I'm not sure I like the API and how it's intertwined with the "default" Connector.

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 ConnectorInterface and will likely use DI to inject this. They probably care little about how the connector works. Neither should they care about any "additional" parameters. They do care about the target (hostname and port). Hence (IMHO) they should not have to take care of connection timeouts.

I'm not sure if the trait belongs in this library

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)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants