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

Don't raise exceptions from within AMQP callbacks #78

Merged
merged 1 commit into from
Mar 29, 2018

Conversation

miha-plesko
Copy link
Contributor

Raising exceptions from within AMQP callbacks (defined in MessagingHandler) prevents qpid_proton gem from closing TCP connection properly which results in file descriptor leakage (see https://issues.apache.org/jira/browse/PROTON-1791). It's a bug in gem (sockets shouldn't leak no matter what) which will be resolved with next release (qpid_proton 0.22.0) in a month, but with this commit we avoid raising exceptions in callbacks to resolve file descriptor problem immediately.

Replacement for #76
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1554771

@miq-bot assign @juliancheal
@miq-bot add_label enhancement,gaprindashvili/yes

/cc @gberginc

@miha-plesko
Copy link
Contributor Author

@juliancheal I apologize for opening the 3rd PR on same topic! We had hard time figuring out what was causing file descriptor leakage. Eventually, with kind and fast help from qpid_proton maintainers we discovered that sockets are leaking because we're raising exceptions in callbacks which is not accepted well by the gem 🔥 💥 🎆 😄

I think this PR should actually be fine since we're not monkey-patching anything. Therefore I kindly ask for a review. This time I do not intend to close it after a day or two 👼

Raising exceptions from within AMQP callbacks (defained in MessagingHandler)
prevents qpid_proton gem from closing TCP connection properly which results in
file descriptor leakage (see https://issues.apache.org/jira/browse/PROTON-1791).
It's a bug in gem (sockets shouldn't leak no matter what) which will be resolved with
next release (qpid_proton 0.22.0) in a month, but with this commit we avoid raising
exceptions in callbacks to resolve file descriptor problem immediately.

Signed-off-by: Miha Pleško <[email protected]>
@miha-plesko miha-plesko force-pushed the remove-raise-from-callbacks branch from a897e9b to acf3949 Compare March 19, 2018 15:15
@miq-bot
Copy link
Member

miq-bot commented Mar 19, 2018

Checked commit miha-plesko@acf3949 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍪

@juliancheal
Copy link
Member

@miha-plesko LGTM, want me to merge?

@miha-plesko
Copy link
Contributor Author

@juliancheal yes please, I'm confident it will work.

@miha-plesko
Copy link
Contributor Author

@juliancheal ping

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

Slick workaround, lgtm

@agrare agrare merged commit 8317791 into ManageIQ:master Mar 29, 2018
@agrare
Copy link
Member

agrare commented Mar 29, 2018

Let's remember to remove this once the gem is fixed

@agrare agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Mar 29, 2018
simaishi pushed a commit that referenced this pull request Apr 3, 2018
Don't raise exceptions from within AMQP callbacks
(cherry picked from commit 8317791)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563361
@simaishi
Copy link
Contributor

simaishi commented Apr 3, 2018

Gaprindashvili backport details:

$ git log -1
commit 57c3a3be118fbf3271853934cd07bd29eff11267
Author: Adam Grare <[email protected]>
Date:   Thu Mar 29 10:45:22 2018 -0400

    Merge pull request #78 from miha-plesko/remove-raise-from-callbacks
    
    Don't raise exceptions from within AMQP callbacks
    (cherry picked from commit 83177915d725198c81c357279e5ca1672fed6aac)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1563361

@miha-plesko miha-plesko deleted the remove-raise-from-callbacks branch August 8, 2018 06:18
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.

5 participants