#3360 proton: save messages when connection fails
Merged by tkopecek. Opened by tkopecek.
tkopecek/koji issue3327  into  master

Download 3360.patch

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

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.

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.

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

It looks to me, that on_settled should work correctly also in this case. So 'context.msgs' should be in valid state.

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

Metadata