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

Catch writer.wait_closed() exception to avoid command hanging. #33 #34

Conversation

Ivashkaization
Copy link

@Ivashkaization Ivashkaization commented Apr 2, 2021

Fixes #33

As asyncio docs says about StreamWriter.write method:

The method attempts to write the data to the underlying socket immediately. If that fails, the data is queued in an internal write buffer until it can be sent.

Also, wait_closed is writing data left in buffer before exiting

However, in library code any exception in wait_closed is ignored:

ansq/ansq/tcp/connection.py

Lines 122 to 128 in cb4c722

assert self._writer is not None
try:
self._writer.close()
if sys.version_info >= (3, 7):
await self._writer.wait_closed()
finally:
pass

And code intended to unhang command located below is never reached

ansq/ansq/tcp/connection.py

Lines 130 to 133 in cb4c722

for future, callback in self._cmd_waiters:
if not future.cancelled():
future.set_exception(ConnectionClosedError("Connection is closed"))
callback is not None and callback(None)

Copy link
Collaborator

@atugushev atugushev left a comment

Choose a reason for hiding this comment

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

Nice catch 👍🏻 Thanks!

@atugushev
Copy link
Collaborator

@GinTR1k it seems python-test workflow is disabled due to lack of activity. Could you please enable it?

image

@GinTR1k
Copy link
Contributor

GinTR1k commented Apr 2, 2021

Hi!

I am no longer the maintainer, sorry. I think @shalakhin would help with that.

@shalakhin
Copy link
Contributor

Hi guys. Accept this PR. Thank you for PR to the project!

@shalakhin shalakhin merged commit ed4424d into list-family:master Apr 21, 2021
@lig
Copy link
Collaborator

lig commented May 5, 2021

@shalakhin could we have a release with this fix?

@shalakhin
Copy link
Contributor

@shalakhin could we have a release with this fix?

Hey! Yes, this week we will release a new version with accepted and merged PRs! Also thanks everyone for contributions!

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.

Hangs on closing connection if nsqd socket is closed during message sending
5 participants