Fixes: https://pagure.io/koji/issue/3327
It looks to me, that on_settled should work correctly also in this case. So 'context.msgs' should be in valid state.
It's dangerous to have bare exceptions like this, especially silently passing them like this. I understand your comment about on_settled, but there is a lot of code that gets called inside these two lines. Also, imagine if anyone ever changes TimeoutHandler in the future to raise more unhandled exceptions. Or imagine we find there are some other errors that we really wish we had known about, etc. For example, https://issues.apache.org/jira/browse/PROTON-2465
pass
on_settled
TimeoutHandler
Ticket #3327 shows a socket.getaddrinfo() failure, presumably a temporary DNS failure. We could narrow this except statement to only catch socket.error. And I think we should log when we hit this, so we know how bad the problem is, rather than silently passing.
socket.getaddrinfo()
except
socket.error
Here is what I recommend instead:
try: container = Container(TimeoutHandler(url, msgs, CONFIG)) container.run() except socket.error as e: LOG.debug('container setup error (%s): %s', url, e)
_send_msgs should always pass. It returns list of un/sent messages so those which were succesfully sent can be deleted from the db queue, so we don't send them twice. We should log every error but I'm not sure if any of these should be terminating.
_send_msgs
Ok. I think we should comment why we're doing this. Like this:
try: container = Container(TimeoutHandler(url, msgs, CONFIG)) container.run() except Exception as e: # It's ok if we don't send messages for any reason. We'll try again later. LOG.debug('container setup error (%s): %s', url, e)
rebased onto fd2b76e0781466139785d841d3f16c45688aad48
Ok, I see that the docs for on_settled says, "This is the point at which it should never be retransmitted," [1] so hopefully unlikely that we'd have an error causing duplicate transmission. I guess the biggest risk there is a bug in our own on_settled handler, but it seems pretty simple and solid.
[1] https://qpid.apache.org/releases/qpid-proton-0.32.0/proton/python/docs/overview.html
:thumbsup:
Metadata Update from @tkopecek: - Pull-request tagged with: testing-ready
pretty please pagure-ci rebuild
Metadata Update from @mfilip: - Pull-request tagged with: testing-done
Commit b94838b7 fixes this pull-request
Pull-Request has been merged by tkopecek
Fixes: https://pagure.io/koji/issue/3327
It looks to me, that on_settled should work correctly also in this case. So 'context.msgs' should be in valid state.