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

If we don't want timeouts we must set the socket's timeout to None #281

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thedrow
Copy link
Member

@thedrow thedrow commented Jun 21, 2019

See https://docs.python.org/3/library/socket.html#socket.socket.settimeout.

This commit was triggered by celery/celery#4876 (comment).
It could fix the issue as well.

@thedrow
Copy link
Member Author

thedrow commented Jun 22, 2019

With this change, the integration tests hang on Travis.

@matusvalo Any idea why?

@matusvalo
Copy link
Member

matusvalo commented Jul 1, 2019

@thedrow, see my comments in the patch. The problem is that when you are using publisher confirm, the publisher checks before publishing if broker sent a message to him. This relies on timeout=0 which this patch has changed semantics to block forever.

@thedrow
Copy link
Member Author

thedrow commented Jul 2, 2019

So what should we do to fix this issue exactly?

Copy link
Member

@matusvalo matusvalo left a comment

Choose a reason for hiding this comment

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

What would change on PR is:

  1. remove internal interfaces changes which breaks current code - having_timeout() method.
  2. Fix all timeouts in public interfaces...

py-amqp/amqp/channel.py

Lines 1686 to 1688 in b36bf88

def _basic_publish(self, msg, exchange='', routing_key='',
mandatory=False, immediate=False, timeout=None,
argsig='Bssbb'):

def blocking_read(self, timeout=None):

In this way it should work. Maybe we can add also integration tests for each timeout input?

@@ -91,7 +92,7 @@ def connect(self):

@contextmanager
def having_timeout(self, timeout):
if timeout is None:
if timeout is None or timeout == 0:
Copy link
Member

Choose a reason for hiding this comment

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

This line of code breaks the following:

py-amqp/amqp/channel.py

Lines 1764 to 1769 in b36bf88

if capabilities.get('connection.blocked', False):
try:
# Check if an event was sent, such as the out of memory message
self.connection.drain_events(timeout=0)
except socket.timeout:
pass

Copy link
Member

Choose a reason for hiding this comment

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

The def having_timeout() method should be not changed because:

  1. it is internal API
  2. we are using it as mentioned above.

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

Successfully merging this pull request may close these issues.

2 participants