Skip to content

Conversation

isoos
Copy link
Contributor

@isoos isoos commented Aug 2, 2021

As we set _closing = true in the line above the if condition, I think the expression should depend on the force parameter.

@isoos isoos requested review from jonasfj and sigurdm August 2, 2021 15:40
@google-cla google-cla bot added the cla: yes label Aug 2, 2021
Future<void> close({bool force = false}) async {
_closing = true;

if (!_closing) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How was this even supposed to work before?

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 think we have never sent the QUIT message.

Copy link
Member

Choose a reason for hiding this comment

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

I think I googled it and figured closing the outgoing stream while continuing to read ought to be just as reasonable.

But I'm okay, sending QUIT, why not..

Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

Future<void> close({bool force = false}) async {
_closing = true;

if (!_closing) {
Copy link
Member

Choose a reason for hiding this comment

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

I think I googled it and figured closing the outgoing stream while continuing to read ought to be just as reasonable.

But I'm okay, sending QUIT, why not..

if (!force) {
// Always send QUIT message to be nice
try {
final quit = command(['QUIT']);
Copy link
Member

Choose a reason for hiding this comment

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

This will throw if _closing = true I guess we need to set it later...

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants