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.

Sign up for GitHub

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

Jump to bottom

Handle thread safety wrt the messaging API in callbacks #3145

Merged
merged 2 commits into from
May 2, 2019
Merged

Handle thread safety wrt the messaging API in callbacks #3145

merged 2 commits into from
May 2, 2019

Conversation

abompard
Copy link
Member

The message callbacks are run in a thread by Twisted, and must therefore call the messaging API with the proper Twisted wrapper to ensure thread safety.

See: https://fedora-messaging.readthedocs.io/en/stable/consuming.html#synchronous-and-asynchronous-calls

@abompard abompard requested a review from a team as a code owner April 11, 2019 10:20
bowlofeggs
bowlofeggs suggested changes Apr 15, 2019
View reviewed changes
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

One suggestion/question and one requested change so far.

requirements.txt Outdated
@@ -6,7 +6,7 @@ colander
cornice>=3.1.0
dogpile.cache
pyasn1-modules # Due to an unfortunate dash in its name, installs break if pyasn1 is installed first
fedora_messaging
fedora_messaging>=1.6.0
feedgen
jinja2
kitchen
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we directly use twisted now, we should also add twisted to this list.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I've added it.

bodhi/server/consumers/composer.py Outdated
# safety.
blockingCallFromThread(
reactor, notifications.publish,
topic="composer.start", msg=dict(agent=agent), force=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be cleaner if we made notifications.publish() do this when passed a new option (perhaps a boolean like run_in_reactor or some better name)? I don't mind this, but it might be easier to remember to do it if it were done in one place and had an easy parameter to select the behaviour.

What do you think? I'm fine with this style if you prefer it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried that at first, but I think it's better that way, because if we do it in notifications.publish() we'll have to handle the case where notifications are delayed after the DB transaction, so that one more layer down where we'll have to pass that flag. And since it's an issue that only affects message callbacks and is pretty specific to the way message callbacks are run, I thought it's more logical to handle these particularities there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool.

@abompard abompard force-pushed the fix-consumers-threading branch 2 times, most recently from 793391d to f63c0bb Compare April 18, 2019 08:45
@abompard abompard force-pushed the fix-consumers-threading branch 2 times, most recently from a6f7dec to 2cbe13f Compare April 23, 2019 08:12
@abompard
Copy link
Member Author

Rebased.

bowlofeggs
bowlofeggs suggested changes May 1, 2019
View reviewed changes
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

We should add a release note that twisted is now required - I'll do this for you, push and approve. Thanks!

@bowlofeggs
Copy link
Contributor 

diff --git a/docs/user/release_notes.rst b/docs/user/release_notes.rst
index 681d1d67..c4ce122f 100644
--- a/docs/user/release_notes.rst
+++ b/docs/user/release_notes.rst
@@ -77,6 +77,7 @@ Dependency changes
 * six is no longer required for the client or server (:issue:`2759`).
 * ``bodhi-server`` now depends on ``bodhi-messages``.
 * kitchen is no longer required.
+* Twisted is now required (:issue:`3145`).
 
 
 Server upgrade instructions

bowlofeggs
bowlofeggs approved these changes May 1, 2019
View reviewed changes
Copy link
Contributor

@bowlofeggs bowlofeggs left a comment

Choose a reason for hiding this comment

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

Thanks!

@bowlofeggs
Copy link
Contributor

Strange, the f29 build failed with rpmdb corruption issues:

14:10:21  �[91merror: rpmdb: BDB0060 PANIC: fatal region error detected; run recovery
14:10:21  �[0m�[91merror: db5 error(-30973) from db->close: BDB0087 DB_RUNRECOVERY: Fatal error, run database recovery
14:10:21  error: rpmdb: BDB0060 PANIC: fatal region error detected; run recovery
14:10:21  error: db5 error(-30973) from db->close: BDB0087 DB_RUNRECOVERY: Fatal error, run database recovery

@bowlofeggs bowlofeggs mentioned this pull request May 1, 2019
Merged
@bowlofeggs
Copy link
Contributor

This failure happened in #3158 as well.

@bowlofeggs
Copy link
Contributor

I've filed https://bugzilla.redhat.com/show_bug.cgi?id=1705265 about the rpmdb corruption issue.

@bowlofeggs
Copy link
Contributor

I've opened #3207 to work around https://bugzilla.redhat.com/show_bug.cgi?id=1705265

@abompard @bowlofeggs
Handle thread safety wrt the messaging API in callbacks
e07fc25 
The message callbacks are run in a thread by Twisted, and must therefore
call the messaging API with the proper Twisted wrapper to ensure thread
safety.

See: https://fedora-messaging.readthedocs.io/en/stable/consuming.html#synchronous-and-asynchronous-calls

Signed-off-by: Aurélien Bompard <aurelien@bompard.org>
@mergify mergify bot merged commit 3dca0e3 into fedora-infra:develop May 2, 2019
@bowlofeggs
Copy link
Contributor

This patch is planned for inclusion in the upcoming 4.0.0 release: #3221

@bowlofeggs
Copy link
Contributor

Bodhi 4.0.0 beta is built and deployed to staging:

https://copr.fedorainfracloud.org/coprs/bowlofeggs/bodhi-pre-release
https://bodhi.stg.fedoraproject.org/

@bowlofeggs bowlofeggs mentioned this pull request May 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bowlofeggs bowlofeggs bowlofeggs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abompard @bowlofeggs