localcontext store is not monkeypatched by eventlet

[Mehdi Abaakouk]

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,

[OpenStack Infra (hudson-openstack)]

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

[Mehdi Abaakouk (sileht)]

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.

[OpenStack Infra (hudson-openstack)]

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

[Terry Yao (yaohaif)]

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

[Sergey Gotliv (sgotliv)]

Terry,

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

[Sergey Gotliv (sgotliv)]

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.

[Flavio Percoco (flaper87)]

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).

[Flavio Percoco (flaper87)]

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

[Mehdi Abaakouk (sileht)]

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 .

[Sergey Gotliv (sgotliv)]

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!

[Mehdi Abaakouk (sileht)]

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.

[Flavio Percoco (flaper87)]

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.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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