localcontext store is not monkeypatched by eventlet

Bug #1288878 reported by Mehdi Abaakouk
22
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Ceilometer
Undecided
Mehdi Abaakouk
oslo.messaging
Critical
Mehdi Abaakouk

Bug Description

Hi,

If oslo.messaging.localcontext is loaded before eventlet monkeypatch the threading module, the localcontext won't works as expected, the following error occurs:

2014-03-06 17:00:34.432 26438 ERROR oslo.messaging.notify.dispatcher [-] Exception during message handling
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher Traceback (most recent call last):
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher File "/vagrant/stack-master/oslo.messaging/oslo/messaging/notify/dispatcher.py", line 87, in _dispatch_and_handle_error
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher return self._dispatch(incoming.ctxt, incoming.message)
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher File "/vagrant/stack-master/oslo.messaging/oslo/messaging/notify/dispatcher.py", line 130, in _dispatch
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher localcontext.clear_local_context()
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher File "/vagrant/stack-master/oslo.messaging/oslo/messaging/localcontext.py", line 60, in clear_local_context
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher delattr(_STORE, _KEY)
2014-03-06 17:00:34.432 26438 TRACE oslo.messaging.notify.dispatcher AttributeError: _oslo_messaging_localcontext_41592ac342124790a928879c3708e651

Cheers,

Mehdi Abaakouk (sileht)
Changed in oslo.messaging:
assignee: nobody → Mehdi Abaakouk (sileht)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo.messaging (master)

Fix proposed to branch: master
Review: https://review.openstack.org/78674

Changed in oslo.messaging:
status: New → In Progress
Mark McLoughlin (markmc)
Changed in oslo.messaging:
importance: Undecided → Critical
Revision history for this message
Mehdi Abaakouk (sileht) wrote :

We don't have the ensure eventlet can monkeypatch module at running. Applications that use eventlet must ensure they have monkeypatched python modules before loading anything else.

So, I will fix ceilometer instead of oslo.messaging.

Changed in oslo.messaging:
status: In Progress → Invalid
Changed in ceilometer:
assignee: nobody → Mehdi Abaakouk (sileht)
Changed in ceilometer:
status: New → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to ceilometer (master)

Reviewed: https://review.openstack.org/79628
Committed: https://git.openstack.org/cgit/openstack/ceilometer/commit/?id=78331c079bd69b51fb3dddd690c280434f533bd7
Submitter: Jenkins
Branch: master

commit 78331c079bd69b51fb3dddd690c280434f533bd7
Author: Mehdi Abaakouk <email address hidden>
Date: Tue Mar 11 10:36:01 2014 +0100

    Eventlet monkeypatch must be done before anything

    This patch ensure eventlet have patched modules before they are loaded
    and used.

    Closes-bug: #1288878

    Change-Id: Ieb1fc252bde94a19fdba14a87c9f97848208bdc5

Changed in ceilometer:
status: In Progress → Fix Committed
Thierry Carrez (ttx)
Changed in ceilometer:
milestone: none → icehouse-rc1
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in ceilometer:
milestone: icehouse-rc1 → 2014.1
Revision history for this message
Terry Yao (yaohaif) wrote :

Do we also need to monkeypatch the "thread=True" as well as "socket=True, select=True"?

Revision history for this message
Sergey Gotliv (sgotliv) wrote :

Terry,

I had to monkeypatch thread module as well in Trove to overcome this problem.

Revision history for this message
Sergey Gotliv (sgotliv) wrote :

Guys,

I think that all usages of clear_local_context in oslo.messaging library must be wrapped
with try/except and just print a warning to the log instead of propagating this error to the caller.
I don't see any particular reason why clear operation should fail especially considering a fact
that caller doesn't aware of localcontext existence and doesn't use it.

Revision history for this message
Flavio Percoco (flaper87) wrote :

I agree with Sergey here. The library's assumption of an existing context is wrong and I don't think blowing this error on user's face is the right thing to do here.

The previous fix attempted by Mehdi tried to set the local context if it didn't exist, I think a more conservative fix is the one proposed by Sergey and I'd be in favour of it (assuming I'm not missing something important/critical here).

Changed in oslo.messaging:
status: Invalid → Confirmed
Revision history for this message
Flavio Percoco (flaper87) wrote :

I moved the bug back to "Confirmed" to start a new discussion on it.

Revision history for this message
Mehdi Abaakouk (sileht) wrote :

I understand that this error message can be confuse user, but that can occurs only because something bad have been done outside the library. By something, I think of threading module not monkeypatched or monkeypatched after the library have been loaded.

oslo.messaging assumes that time and thread are monkey patched in case of eventlet executor. (perhaps that needs to be documented, and perhaps eventlet executor needs to warn if this is not the case).

Also localcontext is public API, and have to work, so try..catch is obviously not a good solution.

I'm not against my first patch, but in case of monkey patching not done at the right moment I prefer to see the code fail on that simple threading.local() issue, than see oslo.messaging stuck somewhere or have race condition .

Revision history for this message
Sergey Gotliv (sgotliv) wrote :

Mehdi,

Today oslo.messaging fails on localcontext.clear_local_context() with totally useless message which makes me guess how is that possible that following code succeeded on set_local_context() but failed on clear_local_context(). Remember that as a user I didn't choose to use eventlet executor (its a default values) and localcontext is used internally by oslo.messaging in that case:

if hasattr(endpoint, method):
                localcontext.set_local_context(ctxt)
                try:
                    return self._do_dispatch(endpoint, method, ctxt, args)
                finally:
                    localcontext.clear_local_context()

All I am saying is that you can't break the flow on clear_local_context(), its a bad user experience. Clear must be safe operation, definitely it can't raise an AttributeError!

Revision history for this message
Mehdi Abaakouk (sileht) wrote :

This use threading.local..., if you got an AttributeError, this is because you have not monkeypatched threading or because monkeypatch have been done after you load oslo.messaging module.

Revision history for this message
Flavio Percoco (flaper87) wrote :

This is a bit tricky thing to work around on. Since this issue is merely caused by a misuse of eventlet's monkey patch, there's not much we, as maintainers of the library, can do. Nonetheless, we could warn the user about the misuse of eventlet by detecting whether monkey patch was called too late and warning the user about it.

This could be done in the eventlet executor and it should provide enough information to teach the user what's the proper way to use oslo.messaging with eventlet.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to oslo.messaging (master)

Fix proposed to branch: master
Review: https://review.openstack.org/140610

Changed in oslo.messaging:
status: Confirmed → In Progress
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to oslo.messaging (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/140616

Mehdi Abaakouk (sileht)
Changed in oslo.messaging:
milestone: none → next-kilo
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to oslo.messaging (master)

Reviewed: https://review.openstack.org/140610
Committed: https://git.openstack.org/cgit/openstack/oslo.messaging/commit/?id=d3e6ea17881bdeefcebccbff691b9084315f2393
Submitter: Jenkins
Branch: master

commit d3e6ea17881bdeefcebccbff691b9084315f2393
Author: Mehdi Abaakouk <email address hidden>
Date: Wed Dec 10 09:33:22 2014 +0100

    Warns user if thread monkeypatch is not done

    This change warns the user that it does have monkeypatched the
    oslo.messaging library correclty.

    Change-Id: Ice80ffc6cbc39919eac4bc0809992daea51b5922
    Closes-bug: #1288878

Changed in oslo.messaging:
status: In Progress → Fix Committed
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix proposed to oslo.messaging (master)

Related fix proposed to branch: master
Review: https://review.openstack.org/147086

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Change abandoned on oslo.messaging (master)

Change abandoned by Mehdi Abaakouk (<email address hidden>) on branch: master
Review: https://review.openstack.org/147086
Reason: wrong change id

Changed in oslo.messaging:
status: Fix Committed → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to oslo.messaging (master)

Reviewed: https://review.openstack.org/140616
Committed: https://git.openstack.org/cgit/openstack/oslo.messaging/commit/?id=01247563f1a00648887954ddc049c036bc9ddb69
Submitter: Jenkins
Branch: master

commit 01247563f1a00648887954ddc049c036bc9ddb69
Author: Mehdi Abaakouk <email address hidden>
Date: Wed Jan 14 09:07:22 2015 +0100

    Deprecates the localcontext API

    We deprecate the localcontext API in favor of the
    oslo.context one.

    Related bug: #1288878

    Change-Id: If6049544e35ae49e41f2771cae858b6d4ebe00cd

To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers