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

Potentially broken _AbstractTransport.__repr__ #361

Closed
blueyed opened this issue May 19, 2021 · 4 comments · Fixed by #431
Closed

Potentially broken _AbstractTransport.__repr__ #361

blueyed opened this issue May 19, 2021 · 4 comments · Fixed by #431

Comments

@blueyed
Copy link

blueyed commented May 19, 2021

_AbstractTransport.__repr__ (or Connection.__repr__) might crash.

In a report/issue from/in Sentry self was reported as "broken repr" in

self._write(s)
.

_AbstractTransport.__repr__:

py-amqp/amqp/transport.py

Lines 100 to 106 in 7300741

def __repr__(self):
if self.sock:
src = f'{self.sock.getsockname()[0]}:{self.sock.getsockname()[1]}'
dst = f'{self.sock.getpeername()[0]}:{self.sock.getpeername()[1]}'
return f'<{type(self).__name__}: {src} -> {dst} at {id(self):#x}>'
else:
return f'<{type(self).__name__}: (disconnected) at {id(self):#x}>'

Connection.__repr__:

py-amqp/amqp/connection.py

Lines 281 to 287 in 7300741

def __repr__(self):
if self._transport:
return f'<AMQP Connection: {self.host}/{self.virtual_host} '\
f'using {self._transport} at {id(self):#x}>'
else:
return f'<AMQP Connection: {self.host}/{self.virtual_host} '\
f'(disconnected) at {id(self):#x}>'

The full traceback:

ConnectionResetError: [Errno 104] Connection reset by peer
  File "celery/worker/loops.py", line 78, in asynloop
    update_qos()
  File "kombu/common.py", line 444, in update
    return self.set(self.value)
  File "kombu/common.py", line 437, in set
    self.callback(prefetch_count=new_value)
  File "celery/worker/consumer/tasks.py", line 43, in set_prefetch_count
    return c.task_consumer.qos(
  File "kombu/messaging.py", line 552, in qos
    return self.channel.basic_qos(prefetch_size,
  File "amqp/channel.py", line 1878, in basic_qos
    return self.send_method(
  File "amqp/abstract_channel.py", line 57, in send_method
    conn.frame_writer(1, self.channel_id, sig, args, content)
  File "amqp/method_framing.py", line 183, in write_frame
    write(view[:offset])
  File "amqp/transport.py", line 352, in write
    self._write(s)
BrokenPipeError: [Errno 32] Broken pipe
  File "kombu/message.py", line 128, in ack_log_error
    self.ack(multiple=multiple)
  File "kombu/message.py", line 123, in ack
    self.channel.basic_ack(self.delivery_tag, multiple=multiple)
  File "amqp/channel.py", line 1391, in basic_ack
    return self.send_method(
  File "amqp/abstract_channel.py", line 57, in send_method
    conn.frame_writer(1, self.channel_id, sig, args, content)
  File "amqp/method_framing.py", line 183, in write_frame
    write(view[:offset])
  File "amqp/transport.py", line 352, in write
    self._write(s)

I have not investigated why that is yet, but figured reporting it already is worth it, since having a proper (non-crashing) repr is more helpful during debugging/investigating issues, of course.

amqp 5.0.6 (but the code is the same in master)
sentry-sdk 1.1.0

@matusvalo
Copy link
Member

Thank you @blueyed for your bug report. Unfortunately, I cannot understand what break in __repr__ from your report. I don't see anything regarding repr in your traceback. Could you be more specific?

@nburlett
Copy link
Contributor

nburlett commented Apr 9, 2024

I have a similar problem. I don't yet understand why the socket is getting disconnected, but when it does the transport __repr__ throws an exception: [Errno 107] Transport endpoint is not connected in amqp.transport ine 119:

dst = f'{self.sock.getpeername()[0]}:{self.sock.getpeername()[1]}'

Because the socket is no currently connected, self.sock.getpeername() will raise an exception.

The __repr__ should not raise an exception here: socket errors are to be expected and attempting to log them shouldn't cause more errors.

One possible fix would be:

try:
    dst = f'{self.sock.getpeername()[0]}:{self.sock.getpeername()[1]}'
except (socket.error) as e:
    dst = f'ERROR: {e}'

@auvipy
Copy link
Member

auvipy commented May 8, 2024

I would be happy to review a PR with this fix and enough unit tests

nburlett added a commit to nburlett/py-amqp that referenced this issue May 21, 2024
If AbstractTransport's self.sock is disconnected, then
self.sock.getpeername() will raise an error in __repr__, which shows up
in backtraces for other upstream errors.

Fix this by catching socket.error in __repr__ and putting the error info
in to the returned repr string.

Fixes celery#361
@nburlett
Copy link
Contributor

nburlett commented May 21, 2024

I just submitted a PR (#431). It passed unit tests on cpython but I don't have pypi installed so that was skipped by tox locally. I wasn't able to the integration tests, as I'm seeing errors like:

docker.errors.ImageNotFound: 404 Client Error for http+docker://localhost/v1.44/images/rabbitmq:tls/json: Not Found ("No such image: rabbitmq:tls")

I've not run tox with docker before, so I may have something misconfigured.

Edit: looks like the github jobs also fail the integration test, albeit with a slightly different error message

nburlett added a commit to nburlett/py-amqp that referenced this issue Jun 3, 2024
If AbstractTransport's self.sock is disconnected, then
self.sock.getpeername() will raise an error in __repr__, which shows up
in backtraces for other upstream errors.

Fix this by catching socket.error in __repr__ and putting the error info
in to the returned repr string.

Fixes celery#361
auvipy pushed a commit that referenced this issue Jun 4, 2024
If AbstractTransport's self.sock is disconnected, then
self.sock.getpeername() will raise an error in __repr__, which shows up
in backtraces for other upstream errors.

Fix this by catching socket.error in __repr__ and putting the error info
in to the returned repr string.

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

Successfully merging a pull request may close this issue.

4 participants