[qpid-proton-cpp] default_container does not implement thread safety at all?

classic Classic list List threaded Threaded
9 messages Options
Fj
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[qpid-proton-cpp] default_container does not implement thread safety at all?

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

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?

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?

----------------

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.

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.

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

  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.

  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.

  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.
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

Andrew Stitcher
On Tue, 2017-03-14 at 01:23 +0200, Fj wrote:
> ... (elided somewhat inflammatory comments)

> Am I missing something obvious,

That's difficult to say (obvious is so subjective!)

The code you are looking at implements the multithreaded API, but is
not multithreaded itself, and yes it is unfortunate that it doesn't do
a thread safe inject, but C++03 has no threading capability and the st
code will compile with 03. Since it's important for us to minimise
dependencies for the proton libraries, this code can't do much better.

In the same code drop you will find multithreaded implementations of
the container API too - This is in the examples/cpp/mt directory, this
code relies on external thread capabilities.

Note that we use the threading abilities of the C++11 and later
standard library, so you will need to compile with a modern compiler to
get mt support.

Note that this does mean that if you want to support *any* variant of
multithreading with the C++ binding (either multiple worker threads for
the container, or running the container in a separate thread to the
rest of your application) you will need C++11 or later to compile the
proton C++ binding library. I don't consider this to be a real problem
at this point. C++11 is supported well enough for our purposes in all
popular C++ compilers, and has been for quite some time - I think over
5 years.

> 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?
>
> 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?

Experimental means that the API is not stable, but is provided so you
can get something done, and hopefully let us know how it goes, and give
us feedback so we can stabilise the API so that it can be released as
no longer experimental.

All this code is being replaced as we speak with a proactor based
implementation of the container API; this should support multiple
worker threads in the container as long as you have C++11 or later. but
only a single thread with C++03.

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

Custom containers essentially go away in the latest version of the API.
Instead of suggesting the user might want to implement their own
container we are providing a proactor based container.

We are providing proactor implementations based in libuv, epoll (for
Linux), iocp (for Windows). It will be possible to write your own
proactor if necessary, but the ones we will provide cover a lot of the
usual implementation landscape.

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

I think you may have a misunderstanding here. Although the run loops
are owned by the container, the unit of serialisation is the
connection. The "bolting on" is to give access from whichever object
you have to a thread safe context to inject work. Even if the inject
happens at the container level, it's convenient to be able to inject
from an event handler based on the object contained in the event,
rather than having to navigate up to the container and then injecting.
You *must* specify the connection that is to serialised, as
*everything* happens per connection, so there is no value in running
code in the run loop without having some way to safely run code in the
connection context.

The container schedule API does run code only in the container context,
with no connection serialisation, but it's not safe to use to refer to
any connection (or lower ) state without injecting into the relevant
connection.

The "event_loop" concept is to indicate the unit of serialisation. In
theory this could be any level of the object tree. I agree that
"event_loop" is a somewhat confusing name to indicate the serialisation
unit, but we couldn't think of a better one - probably we need to make
the doc clearer.


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

For immediate trying the example mt container is probably your best
bet. In the near future the proactor based container is "the future"
(tm).

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

I don't think the C code has any locking at all. The only way to run it
thread safely is to dedicate a thread to its run loop and poke it with
pn_reactor_wakeup() - this is thread safe.

However we are planning to deprecate the reactor (although not
immediately as the python binding still uses it) and now consider the
proactor its replacement.
>
>   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.

I think this used the same model as the C code - stash something and
then
>
>   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.

Hope this clears up some questions.

Andrew


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

aconway.rh
In reply to this post by Fj
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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

Adel Boutros
Hello,

As Alan said, we have for example "abused" the schedule in C++. Today we run the container on another thread and we have a shared queue between the container threads and the other threads. We call schedule very frequently with a delay of 0. Whenever the container thread wakes, it checks the queue to send events then calls schedule again. The access to the queue is protected by locks.

The downside is that the container thread abuses a full CPU but we can limit this by using a delay of higher than 0 on schedule.

Regards,
Adel

________________________________
From: Alan Conway <[hidden email]>
Sent: Wednesday, March 15, 2017 10:51:24 PM
To: [hidden email]
Subject: Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

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: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

aconway.rh
On Wed, 2017-03-15 at 22:32 +0000, Adel Boutros wrote:

> Hello,
>
> As Alan said, we have for example "abused" the schedule in C++. Today
> we run the container on another thread and we have a shared queue
> between the container threads and the other threads. We call schedule
> very frequently with a delay of 0. Whenever the container thread
> wakes, it checks the queue to send events then calls schedule again.
> The access to the queue is protected by locks.
>
> The downside is that the container thread abuses a full CPU but we
> can limit this by using a delay of higher than 0 on schedule.
>
> Regards,
> Adel
>

And to be clear this is abuse to hack out of a short-term hole - we
will have a properly behaved solution soon.

> ________________________________
> From: Alan Conway <[hidden email]>
> Sent: Wednesday, March 15, 2017 10:51:24 PM
> To: [hidden email]
> Subject: Re: [qpid-proton-cpp] default_container does not implement
> thread safety at all?
>
> 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: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

Venkat B
In reply to this post by Fj
Good day,

---------- Forwarded message ----------
> From: Alan Conway <[hidden email]>
> To: [hidden email]
> Cc:
> Bcc:
> Date: Wed, 22 Mar 2017 14:09:00 -0400
> Subject: Re: [qpid-proton-cpp] default_container does not implement thread
> safety at all?
>
[snip]
>
> And to be clear this is abuse to hack out of a short-term hole - we
> will have a properly behaved solution soon.
>
>
Do you have something that I could test?
The code in examples/cpp/mt does not compile out of the box on 0.17.0 and
in case it is being redesigned anyway then I would prefer to work with the
new API.

Regards,
Venkat
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

aconway.rh
On Fri, 2017-03-24 at 14:06 +0530, Venkat B wrote:

> Good day,
>
> ---------- Forwarded message ----------
> > From: Alan Conway <[hidden email]>
> > To: [hidden email]
> > Cc:
> > Bcc:
> > Date: Wed, 22 Mar 2017 14:09:00 -0400
> > Subject: Re: [qpid-proton-cpp] default_container does not implement
> > thread
> > safety at all?
> >
>
> [snip]
> >
> > And to be clear this is abuse to hack out of a short-term hole - we
> > will have a properly behaved solution soon.
> >
> >
>
> Do you have something that I could test?
> The code in examples/cpp/mt does not compile out of the box on 0.17.0
> and
> in case it is being redesigned anyway then I would prefer to work
> with the
> new API.
>

I defer to astitcher on the current state of the examples. The API
isn't changing significantly, the code in mt/broker.cpp will still be
correct with only minor changes if any. 

The code in epoll_container.cpp will also still be basically unchanged
but writing a custom container will no longer be necessary for most
cases: the default_container will be thread-safe and we will have
epoll-native, iocp-native and libuv flavours to cover most needs.



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Fj
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

Fj
By the way, if anyone is interested, as a stop-gap solution I ended up just
implementing a timer-based callback reenqueuing itself every 50ms and
pulling callbacks from a properly synchronized queue and executing them on
the worker thread. It's not even that bad performance-wise, it just adds
those 50/2 ms of latency on average.

A better solution would be to do the same thing the Python binding does,
expose a Selectable interface and use it to wake up the reactor thread.

And by the way there's a thing that you might want to consider: there's a
bunch of use cases that don't need inter-thread synchronization, for
example, single-threaded workers, or multi-threaded workers that only need
to know when to stop and they know it from the socket being closed. In
those cases exposing a thread-safe Container.inject(callback) is a total
waste, because it means that everything else must constantly take locks for
no reason.

Maybe in the spirit of "you don't pay for what you don't use" the way
Python binding does it is actually fundamentally better, with an
EventInjector thingie that you can add to your Container if you want and
then call injector.inject(callback) instead of container.inject?

On Fri, Mar 24, 2017 at 3:30 PM, Alan Conway <[hidden email]> wrote:

> On Fri, 2017-03-24 at 14:06 +0530, Venkat B wrote:
> > Good day,
> >
> > ---------- Forwarded message ----------
> > > From: Alan Conway <[hidden email]>
> > > To: [hidden email]
> > > Cc:
> > > Bcc:
> > > Date: Wed, 22 Mar 2017 14:09:00 -0400
> > > Subject: Re: [qpid-proton-cpp] default_container does not implement
> > > thread
> > > safety at all?
> > >
> >
> > [snip]
> > >
> > > And to be clear this is abuse to hack out of a short-term hole - we
> > > will have a properly behaved solution soon.
> > >
> > >
> >
> > Do you have something that I could test?
> > The code in examples/cpp/mt does not compile out of the box on 0.17.0
> > and
> > in case it is being redesigned anyway then I would prefer to work
> > with the
> > new API.
> >
>
> I defer to astitcher on the current state of the examples. The API
> isn't changing significantly, the code in mt/broker.cpp will still be
> correct with only minor changes if any.
>
> The code in epoll_container.cpp will also still be basically unchanged
> but writing a custom container will no longer be necessary for most
> cases: the default_container will be thread-safe and we will have
> epoll-native, iocp-native and libuv flavours to cover most needs.
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [qpid-proton-cpp] default_container does not implement thread safety at all?

aconway.rh
On Fri, 2017-05-26 at 03:06 +0300, Fj wrote:
> By the way, if anyone is interested, as a stop-gap solution I ended
> up just
> implementing a timer-based callback reenqueuing itself every 50ms and
> pulling callbacks from a properly synchronized queue and executing
> them on
> the worker thread. It's not even that bad performance-wise, it just
> adds
> those 50/2 ms of latency on average.

That's the right workaround. The new C++ API has thread-safe work-
queues that will allow you to have a function executed in the proper
context which will be more efficient and more flexible than using the
timer callbacks. Behind the scenes it uses whatever is efficient on the
underlying IO platform (linux eventfds, libuv async_send etc.) to do
the wakeup.
 

>
> A better solution would be to do the same thing the Python binding
> does,
> expose a Selectable interface and use it to wake up the reactor
> thread.
>
> And by the way there's a thing that you might want to consider:
> there's a
> bunch of use cases that don't need inter-thread synchronization, for
> example, single-threaded workers, or multi-threaded workers that only
> need
> to know when to stop and they know it from the socket being closed.
> In
> those cases exposing a thread-safe Container.inject(callback) is a
> total
> waste, because it means that everything else must constantly take
> locks for
> no reason.

+1, those are all good observations. Look at the C proactors to see how
we are implementing this. Suggestions welcome. At the C level all you
can do is trigger a wakeup, to keep the C layer easy to re-implement.
The C++ binding will use the basic wakeup provided by C, and add
programmer convenience in the form of work queues so you can not only
trigger a wakeup but provide a function to be executed.

>
> Maybe in the spirit of "you don't pay for what you don't use" the way
> Python binding does it is actually fundamentally better, with an
> EventInjector thingie that you can add to your Container if you want
> and
> then call injector.inject(callback) instead of container.inject?

That's roughly how it works: there are a set of "contexts", the
container is just one of them. Like selectables but simpler, providing
only "wakeup" not read/write. Each connection, listener and the
container have a context, we're thinking about how to expose the
concept so the user can create contexts of their own for syncing
application data structures.

>
> On Fri, Mar 24, 2017 at 3:30 PM, Alan Conway <[hidden email]>
> wrote:
>
> > On Fri, 2017-03-24 at 14:06 +0530, Venkat B wrote:
> > > Good day,
> > >
> > > ---------- Forwarded message ----------
> > > > From: Alan Conway <[hidden email]>
> > > > To: [hidden email]
> > > > Cc:
> > > > Bcc:
> > > > Date: Wed, 22 Mar 2017 14:09:00 -0400
> > > > Subject: Re: [qpid-proton-cpp] default_container does not
> > > > implement
> > > > thread
> > > > safety at all?
> > > >
> > >
> > > [snip]
> > > >
> > > > And to be clear this is abuse to hack out of a short-term hole
> > > > - we
> > > > will have a properly behaved solution soon.
> > > >
> > > >
> > >
> > > Do you have something that I could test?
> > > The code in examples/cpp/mt does not compile out of the box on
> > > 0.17.0
> > > and
> > > in case it is being redesigned anyway then I would prefer to work
> > > with the
> > > new API.
> > >
> >
> > I defer to astitcher on the current state of the examples. The API
> > isn't changing significantly, the code in mt/broker.cpp will still
> > be
> > correct with only minor changes if any.
> >
> > The code in epoll_container.cpp will also still be basically
> > unchanged
> > but writing a custom container will no longer be necessary for most
> > cases: the default_container will be thread-safe and we will have
> > epoll-native, iocp-native and libuv flavours to cover most needs.
> >
> >
> >
> > -----------------------------------------------------------------
> > ----
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...