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

Rename close() to quit() and use promises for quit() method #65

Merged
merged 2 commits into from
Jul 5, 2018

Conversation

clue
Copy link
Contributor

@clue clue commented Jul 4, 2018

This PR renames the existing close() method to quit() and uses promises for this method instead.

// old
$connection->close(function () {
    echo 'closed';
});

// new
$connection->quit()->then(function () {
    echo 'closed';
});

This PR also includes relevant documentation and tests. The old close() method definition was somewhat inconsistent and under-documented.

Builds on top of #64
Refs #4

@clue
Copy link
Contributor Author

clue commented Jul 5, 2018

For the reference: The method was renamed from close() to quit() to emphasize this method actually sends a "quit" command over the wire once all queued commands are done (it waits for all queued commands, which is also why it now returns a promise). It does not force-close the connection like the stream close() method commonly used in @reactphp does. I'm planning to add a new close(): void method which immediately force-closes the underlying connection and rejects all outstanding commands in a follow-up PR 👍

*
* @return void
* @throws Exception if the connection is not initialized or already closed/closing
* @return PromiseInterface Returns a Promise<true,Exception>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd personally keep the "return" value as void or is there a reason this now resolves with true?

Copy link
Contributor Author

@clue clue Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! Updated to resolve with a void value instead. I'll make sure to file a similar PR which makes sure ping() also resolves with a void value instead of boolean true (refs #63). :shipit:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make sure to file a similar PR which makes sure ping() also resolves with a void value instead of boolean true (refs #63). :shipit:

See #66 :shipit:

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.

3 participants