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

Refactored close procedures to dispatch Errors for all components #55

Merged

Conversation

tvoinarovskyi
Copy link
Contributor

Basically now it will always raise appropriate exceptions for any public calls after closure. Added ClientClosed and ServerClosed exceptions. Maybe they are a little to grained, your thoughts?

Now we can write:

@asyncio.coroutine
def do_stuff(connection):
    queue = yield from declare_queue(connection)
    consumer = yield from queue.consume(lambda x: print(x), no_ack=True)
    yield from asyncio.sleep(1)
    queue.publish(some_message)

connection = yield from connect()
while True:
    try:
        yield from consume(connection)
    except asynqp.exceptions.AlreadyClosed:
        connection = yield from reconnect()

and it will raise no matter where connection is lost.

Depends on #54. Please review that firts. I kinda split them in a way )

@tvoinarovskyi
Copy link
Contributor Author

Rebased

@tvoinarovskyi
Copy link
Contributor Author

Bump. I would really appreciate feedback on this, as it's a stopper for #34

@benjamin-hodgson
Copy link
Owner

Apologies, I haven't had time to review this. I'll try and do it ASAP

self.close()

def close(self):
assert not self._closed, "Why do we close it 2-ce?"
Copy link
Owner

Choose a reason for hiding this comment

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

Is this possible? If a user could provoke this condition by (ab)using the API, we should explicitly throw an exception. (If you believe it to be impossible, then an assertion is fine.)

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 it's not possible. We didn't specify protocol as part of public API in docs.

taras-doba-ua and others added 2 commits November 3, 2015 21:46
… good grained exceptions.

Changed Exceptions hierarchy to:
AMQPError
  AMQPConnectionError
    ConnectionLostError
    ClientConnectionClosed
    ServerConnectionClosed
  AMQPChannelError
    ClientChannelClosed
    ContentTooLarge
    NotFound
    NoConsumers
    NotAllowed
    ...
@tvoinarovskyi tvoinarovskyi force-pushed the reader_refactor branch 3 times, most recently from b8e5df3 to f2d9c9f Compare November 3, 2015 21:58
@tvoinarovskyi
Copy link
Contributor Author

Squashed and cleaned code. Removed some unneeded changes, so they don't bother in review.
The idea to use PoisonPill for closing was quite helpful, I like this one more.

@tvoinarovskyi
Copy link
Contributor Author

bump again

benjamin-hodgson pushed a commit that referenced this pull request Nov 28, 2015
Refactored close procedures to dispatch Errors for all components
@benjamin-hodgson benjamin-hodgson merged commit 98d1d2b into benjamin-hodgson:master Nov 28, 2015
@tvoinarovskyi tvoinarovskyi deleted the reader_refactor branch December 2, 2015 16:22
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.

3 participants