Added Monitor class to Connection. #105

Merged
merged 2 commits into from Apr 5, 2017

Conversation

Projects
None yet
2 participants
Collaborator

petevg commented Apr 4, 2017

Calling Monitor.status allows a client to determine whether the
connection is "connected", "disconnected" or in an "error" state, and
take appropriate action as a result.

This is meant to address the issue of watchers failing silently when the
websocket connection goes away due to a network issue.

@tvansteenburgh @johnsca @abentley Addresses #99. WIP because I want to write some matrix code that uses this and validate that it actually solves my problem. Edit: no longer WIP, as this works quite nicely in matrix :-)

petevg added some commits Apr 4, 2017

Added Monitor class to Connection.
Calling Monitor.status allows a client to determine whether the
connection is "connected", "disconnected" or in an "error" state, and
take appropriate action as a result.

This is meant to address the issue of watchers failing silently when the
websocket connection goes away due to a network issue.

@petevg petevg changed the title from [WIP] Added Monitor class to Connection. to Added Monitor class to Connection. Apr 4, 2017

@petevg petevg referenced this pull request in juju-solutions/matrix Apr 4, 2017

Merged

Matrix now fails rather than hangs when disconnected. #121

johnsca approved these changes Apr 5, 2017

LGTM.

Do you know if the AllWatcher task goes to done when the connection drops? If not, it would be good to tie this in to that so that the task doesn't sit there waiting for a response that will never come. Actually, an Event interface for detecting connection drop similar to model._watch_received (but public) might be nice.

Collaborator

petevg commented Apr 5, 2017

@johnsca if you drill down the AllWatcher task ends up repeatedly calling connection.recv (via Type.rpc, via AllModelWatcherFacade.Next), which, thanks to a change you made, raises an Exception if the connection unexpectedly closes.

So the future should make it into the "done" state, with an .exception on it. I'm going to go ahead and merge this :-)

@petevg petevg merged commit f93420d into master Apr 5, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment