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

Fix #535, allowing the expected reply_code and reply_text to be passed to on_close_callback #537

Merged
merged 1 commit into from
Apr 29, 2015

Conversation

vitaly-krugl
Copy link
Member

Pass expected reply_code and reply_text from method frame to Connection._on_disconnect from Connection._on_connection_closed. Fixes #535

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.47%) to 69.15% when pulling 09d2ddd on vitaly-krugl:fix-closing-args into 409670b on pika:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 72.53% when pulling 09d2ddd on vitaly-krugl:fix-closing-args into 409670b on pika:master.

@vitaly-krugl
Copy link
Member Author

This fixes #535. Need to re-run the build, as it failed while github was under severe DDoS attack.

@gmr
Copy link
Member

gmr commented Apr 13, 2015

Thanks for the PR, I'll take a look ASAP.

…me to Connection._on_disconnect from Connection._on_connection_closed
@coveralls
Copy link

Coverage Status

Coverage decreased (-3.38%) to 69.24% when pulling 0c9be99 on vitaly-krugl:fix-closing-args into 409670b on pika:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 72.63% when pulling 0c9be99 on vitaly-krugl:fix-closing-args into 409670b on pika:master.

@vitaly-krugl
Copy link
Member Author

@gmr, I removed blocking_connection.py changes from this PR that overlapped/collided with changes in PR #542.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 72.63% when pulling 0c9be99 on vitaly-krugl:fix-closing-args into 409670b on pika:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 72.63% when pulling 0c9be99 on vitaly-krugl:fix-closing-args into 409670b on pika:master.

@robochat
Copy link

There's also another problem with this part of the code though. I always see a warning message from the BaseConnection._adapter_disconnect() method (actually _check_state_on_disconnect()), that says

       WARNING -  ... -  Unknown state on disconnect: 6

connection_state = 6 meaning CONNECTION_CLOSING which makes sense since _adapter_disconnect() is called before Connection._on_disconnect() (which sets the connection_state as closed). It almost seems like in _on_connection_closed() we should be disconnecting the adapter after we run _on_disconnect() ie.

        # Invoke a method frame neutral close
        self._on_disconnect(self.closing[0], self.closing[1])

        # If this did not come from the connection adapter, close the socket
        if not from_adapter:
            self._adapter_disconnect()

But there are probably issues with swapping the order of the calls that I'm not aware of.

@vitaly-krugl
Copy link
Member Author

Thanks @robochat, I will see about cleaning up the warning.

@gmr
Copy link
Member

gmr commented Apr 29, 2015

Will merge with the intention of doing a full regression test before releasing.

gmr added a commit that referenced this pull request Apr 29, 2015
Fix #535, allowing the expected `reply_code` and `reply_text` to be passed to `on_close_callback`
@gmr gmr merged commit 64b3770 into pika:master Apr 29, 2015
@vitaly-krugl
Copy link
Member Author

@robochat, would you mind filing your feedback about the warning "Unknown state on disconnect: 6" as an Issue in pika? Thx!

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

Successfully merging this pull request may close these issues.

Connection._on_connection_closed() loses reply_code and reply_text before calling _on_disconnect
4 participants