Replace listen() call with URIs passed to constructor#61
Merged
clue merged 4 commits intoreactphp:masterfrom Jan 27, 2017
Merged
Replace listen() call with URIs passed to constructor#61clue merged 4 commits intoreactphp:masterfrom
clue merged 4 commits intoreactphp:masterfrom
Conversation
This fixes the temporal dependency design flaw for the most part, because a Server now always represents a listening Socket (which may possibly be in a closed state)
jsor
reviewed
Jan 17, 2017
src/Server.php
Outdated
| $this->loop->removeStream($this->master); | ||
| fclose($this->master); | ||
| $this->removeAllListeners(); | ||
| if (is_resource($this->master)) { |
Member
There was a problem hiding this comment.
Minor nitpicking (and personal preference): I'd like to see an early return here.
if (!is_resource($this->master)) {
return;
}
$this->loop->removeStream($this->master);
fclose($this->master);
$this->removeAllListeners();
jsor
approved these changes
Jan 17, 2017
WyriHaximus
approved these changes
Jan 23, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The current socket API exhibits a temporal dependence, i.e. it's not safe to call
listen()orshutdown()multiple times on the same instance and call order is actually significant. (see #19)This PR replaces the
listen($port, $host)API with URIs passed to the constructor.This means the Server now always represents a listening Socket (which may
possibly be in a closed state).
Serverclass accepts a single port parameter instead of a full URI and assumes a localhost socket in this case for better compatibility with previous releases.This is obviously a BC break from a consumer perspective, but for the most part this is easily patched like this:
Also, empirical evidence suggests many places simply consume a
ServerInterfacein order to react to theconnectionevent - this very common API is not affected.This means that the
Serverclass now starts listening immediately and a consumer can no longer defer thelisten()call. I personally consider this a good thing (poka yoke) as I know many people have stumbled over this. Also, this API is now somewhat similar to Node'snet.createServer()which also starts up a listening server (vs. the older explicitserver.listen()call).If you want to review this PR and don't want to mess with all these details, I'd suggest taking a look at the examples (and their minimal diff). The previous CLI accepted merely an optional port, the new CLI also accepts full listening addresses, for example like this:
Closes #19
Supersedes / closes #6, refs reactphp-legacy/socket-client#74, reactphp/reactphp#199 and #34