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

Clean up temporal dependence #19

Closed
clue opened this issue Jun 7, 2015 · 6 comments · Fixed by #61
Closed

Clean up temporal dependence #19

clue opened this issue Jun 7, 2015 · 6 comments · Fixed by #61
Assignees
Milestone

Comments

@clue
Copy link
Member

clue commented Jun 7, 2015

The current socket API exhibits a temporal dependence:

$server = new Server($loop);
$server->listen(1234);
$server->getPort();
$server->on('connection', function () { });
$server->shutdown();
  • What happens if you call listen() twice?
    It's tempting to think that this will allow you to listen on two addresses – it does not.
  • What happens if you call shutdown() twice or before calling listen()?
    Errors out – should probably be ignored
  • What happens if you call getPort() before calling listen() or after calling shutdown()?
    Errors out – should probably be ignored
  • What happens if you call listen() after calling shutdown()?
    Should work, but makes for an awkward API.

Afaict most of this boils down to this:

  • What does a Server instance actually represent?
    Honestly, I'm not sure how to answer this. Once listen() has been called, it represents a "server socket", but until then?

Afaict most of these issues can be avoided if a Server instance always were to represent a "server socket" (which is possibly in a closed state). This means that the listen call would have to be part of the construction.

One possible API could look like this:

$server = $api->createServer(1234);
$server->getPort();
$server->on('connection', function() { });
$server->shutdown();
@clue
Copy link
Member Author

clue commented Jan 15, 2017

Progress update: I've started looking into fixing the temporal dependence, but it looks like it will require a BC break.

As such, I've filed documentation for the current (broken) behavior via #60 first. This allows us to document the changed behavior for the upcoming v0.5.0 release.

@clue clue self-assigned this Jan 15, 2017
@jsor
Copy link
Member

jsor commented Jan 16, 2017

Note, that when start listening in the constructor, there might be a small timeframe the server can emit events before any listener is registered.

@jsor
Copy link
Member

jsor commented Jan 16, 2017

Regarding my previous comment: This might be theoretical, but it should be clear that the consumer will no longer be able to control when to start listening.

$server = new Server($loop);
longOperationSettingUpListener()->then(function($listener) use ($server) {
    $server->on('connection', $listener);
    $server->listen(1234);
});

@jsor
Copy link
Member

jsor commented Jan 16, 2017

A alternate solution would be to require port and host in the constructor and change double calls to listen and shutdown to be either no-ops or consider them errors by throwing an exception.

$server = new Server($loop, 1234);
$server->getPort(); // Port now always available
$server->shutdown(); // Either no-op or throws an exception
$server->listen();
$server->listen(); // Either no-op or throws an exception
$server->on('connection', function () { });
$server->shutdown();
$server->shutdown(); // Either no-op or throws an exception

@clue
Copy link
Member Author

clue commented Jan 17, 2017

As such, I've filed documentation for the current (broken) behavior via #60 first. This allows us to document the changed behavior for the upcoming v0.5.0 release.

This is now merged. See also #61 for my follow-up PR which fixes the temporal dependency. This should also address all of the issues you've mentioned, but I'll comment on each here for completeness:

Note, that when start listening in the constructor, there might be a small timeframe the server can emit events before any listener is registered.

I believe this to be a theoretical issue only, as this would have a significant effect on the React\Stream\Stream class as well. Both classes call $loop->addReadStream() during their ctor which should register the event listener for future events. I couldn't find anything explicit about this, but at least theoretically, the loop could invoke this callback immediately, i.e. during the method call and even before returning from it. We have yet to see this happen in practice, because this would also break other assumptions.

This might be theoretical, but it should be clear that the consumer will no longer be able to control when to start listening.

Nice find, this is correct 👍 I've explicitly used this example as part of the above PR and believe this is actually a non-issue, aka. a good thing.

[…] change double calls to listen and shutdown to be either no-ops or consider them errors by throwing an exception.

We could certainly do so 👍 I aimed for a similar step first but realized that while we could fix the errors emitted from these functions (which makes perfect sense on its own), this won't fix the temporal dependence, i.e. we still have an explicit execution order. Also, this is still a BC break with existing behavior, so we would have to bump our major version anyway. I thus prefer using this required BC break to clean this up while we're at it :shipit:

@jsor
Copy link
Member

jsor commented Jan 17, 2017

Since this is a BC break anyway, i'm 👍.

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

Successfully merging a pull request may close this issue.

2 participants