qpid-users mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alan Conway <acon...@redhat.com>
Subject Re: [qpid-proton-cpp] default_container does not implement thread safety at all?
Date Wed, 15 Mar 2017 21:51:24 GMT
On Tue, 2017-03-14 at 01:23 +0200, Fj wrote:
> Hello, I'm mightily confused.
> 
> There's
> https://qpid.apache.org/releases/qpid-proton-0.17.0/proton/cpp/api/md
> _mt.html
> that says in no uncertain terms that I can use event_loop.inject() to
> execute a callback on the thread that runs the event loop. There's an
> examples/cpp/mt/broker.cpp linked from the above that's supposed to
> be a
> thread safe implementation of a thread pool running a bunch of
> containers.
> 
> But after "grep inject" and carefully sifting through the results, it
> seems
> that after some indirection it all ends up calling
> 
> // FIXME aconway 2016-06-07: this is not thread safe. It is
> sufficient for
> using
> // default_container::schedule() inside a handler but not for
> inject() from
> // another thread.
> bool event_loop::impl::inject(void_function0& f) {
>     try { f(); } catch(...) {}
>     return true;
> }
> 
> What the hell? Blank exception suppression is just the icing on the
> cake,
> nothing here even resembles doing the actual hard thing: telling the
> reactor to inject a new custom event in a threadsafe manner.

Guilty as charged mlud. This was work in progress when we started to
redo the guts in a more profound way. The Real Thing is coming - it is
available in C now as pn_proactor and the C++ binding is being
refactored on top of it.

> Am I missing something obvious, or is the documentation there, and in
> other
> places, and the example, chemically pure wishful thinking, like, it
> would
> be pretty nice if we supported multithreading, and it would work in
> such
> and such ways then, but unfortunately we don't, as a matter of fact?

The state of the release is probably not clearly documented: the C++ MT
interfaces *allow* you to build an MT app that replaces the
proton::container (as demoed in the mt_broker) BUT the
proton::container itself is still not multithreaded. It has a
compatible API because it will be in future.

> If so, maybe you gals and guys should fix the documentation, and not
> just
> with "experimental" but with "DOESN'T WORK AT ALL" prominent on the
> page?

I agree the doc is not clear, next release it should work as expected
an not as obscurely caveatted.

> On more constructive notes:
> 
> 1) do people maybe use some custom containers, similar to the
> epoll_container in the examples (which doesn't support schedule()
> though, I
> have to point out)? If so, I much desire to see one.

Yes, but the plan is to fix the non-custom container to be fully
thread-safe so you don't have to unless you have special needs. You can
also implement directly in C if you have very special needs.

> 2) why do you attempt to bolt on your event_loop thing to Connection
> and
> below when the only thing that has the run() method is Container,
> therefore
> that's the scope that any worker thread would have? It's the
> Container that
> should have the inject() method. Underlying C API notwithstanding.

Nope: the threading model is seralized-per-connection, so functions
that operate only on connection state and are triggered from an event
handler don't need to be locked. Only interactions that cross
connections (e.g. one connection queues something a subscriber on
another connection needs to wake up) need sync. This is demonstrated
with the mt_broker stuff. That's why injecting is per-connection.

> 3) What's the best way forward if I really have to have a threadsafe
> container that I can inject some event into threadsafely:

The example demonstrates how you can do it, but is incomplete as you
point out. Soon it will be built in.

>   3a) does the C API support thread safety? Like, the whole problem
> is
> touching the reactor's job queue or whatever it has with taking an
> appropriate lock, and it must take that same lock itself for it to
> work.

It does now, see the pn_proactor, there are examples. The C reactor is
fundamentally and forever thread-unsafe and horribly broken in all ways
with respect to concurrency. We started to introduce MT at the C++
layer and work around the reactor - proton::container lagged because it
is so tied to the reactor. 

Finally we decided to replace the pn_reactor entirely with the
pn_proactor in C - this is designed to be concurrent and provide for
replaceable IO. At this point the proactor exists and has initial
implementations (libuv done, native iocp + epoll nearly so) but we are
still working on replacing the reactor. It's been a big job.

> 
>   3b) I checked out the Python API, they use a mock EventInjector
> pollable
> thingie instead of reaching inside the reactor and telling it add an
> event.

Yep, the python API is due for an overhaul also but works for now as
far as python threads are concerned. Ruby likewise. 

>   3c) epoll example does yet another thing apparently (though I'm not
> sure
> it works because it's not covered by any tests): just tell the
> reactor to
> wake up and process the queued job list in your handler.

The reactor is a dead-end threading wise, hence the kerfuffle.

Let us know more about what you are trying to do and we can see if
early access to anything in progress might help you, or if we can find
you some workarounds based on the existing release. Others have worked
around this in the existing container by abusing schedule() and using
some application level locks.






---------------------------------------------------------------------
To unsubscribe, e-mail: users-unsubscribe@qpid.apache.org
For additional commands, e-mail: users-help@qpid.apache.org


Mime
View raw message