ARMmbed / mbed-events

DEPRECATED! This project has moved to mbed-os
https://github.com/ARMmbed/mbed-os/tree/master/events
Apache License 2.0
11 stars 6 forks source link

Remove the EventLoop class #14

Closed geky closed 8 years ago

geky commented 8 years ago

The EventLoop was an interesting concept: the combination of an EventQueue and a Thread. The idea was that the EventLoop would provide a convenient coupling of these two concepts for use in module boundaries, and the EventLoop could abstract out some of the complexities with running an event queue in a thread.

However, with the addition of the chain function, event queues can be easily composed without threads or indirect references to event queues. Threads can still be spawned dynamically in default-constructors, although the overhead is much more explicit and tangible.

Additionally, it turned out there weren't that many complexities with running an event queue in a thread.

There were, surprisingly, several problems with just passing the EventQueue::dispatch function to mbed's Thread constructor.

  1. Split EventQueue::dispatch into overloaded functions

    Apparently, default parameters don't create another target for infering member-function-pointers.

  2. Remove problematic template overloads in Thread constructor

    Issue is actually here https://github.com/ARMmbed/mbed-os/pull/2395. The indirection in the nested callback templates prevented correct overload resolution and let to template-expansion errors.

  3. Exposed break_dispatch

    Allows threaded event queues to shutdown cleanly.

  4. Renamed get_tick to tick

    To match the C api

  5. Renamed DEFAULT_QUEUE_SIZE to EVENTS_QUEUE_SIZE

    To avoid possible name collisions

With these changes, this code:

EventLoop loop;
loop.start();
loop.call(F);
loop.stop();

Becomes:

EventQueue queue;
Thread thread;
thread.start(&queue, &EventQueue::dispatch);
queue.call(F);
queue.break_dispatch();
thread.join();

Except now with 182 less lines to maintain

pan- commented 8 years ago

What is break_ ? Looks like a protected or private member function under certain conventions.

geky commented 8 years ago

@pan-, you're right the break_ looks a bit odd. break is a good name for the operation (for example libevent's event_base_loopbreak), but unfortunately conflicts with the break keyword, thus the trailing underscore.

Is there an alternative name you think we should use instead?

pan- commented 8 years ago

@geky nope, I was just thinking of the trailing underscore but I forgot about the break keyword. If it stop the event loop maybe stop like in libuv ?

kjbracey commented 8 years ago

"cancel", like pthreads? (Okay, it's not a thread per se, but the effect is the same - "sends a cancellation request").

kjbracey commented 8 years ago

I don't think "break" is a good name, as AFACT it doesn't do what libevent's loopbreak or C break does - it doesn't stop as soon as possible. It's closer to libevent's loopexit(0) - exit after processing all active events. (Not that that's any better as a C analogue).

So I'd suggest any of "cancel", "exit" or "loopexit".

geky commented 8 years ago

@kjbracey-arm, you make a good point, although even with the slightly different semantics, break is still used to indicate that the running loop should terminate, and has the benefit of being hard to confuse for a different operation.

I'm not a fan of exit, since it describes that action taken by the loop, not the action taken by the caller, and unfortunately, cancel is already taken for canceling individual events.

I'm leaning towards @pan-'s stop, although if we want to get rid of the underscore, we could use loopbreak?

kjbracey commented 8 years ago

I'm okay with stop (or loopstop).

But seeing libevent has chosen a meaning for (loop)break I'm loathe to use it for something else libevent already has a different name for.

The loop would be a bit odd though in that you're using "dispatch" instead of "loop" as your base executor, unlike libevent.

How about "stop_dispatch" or "dispatch_stop" or something along those lines?

geky commented 8 years ago

Instead I should have refered to ev_break present in libev, which matches the behaviour of uv_run and event_base_loopexit(0). I don't believe there is enough consensus with existing event loops that we need to worry about misinterpretation, the user will probably need to refer to the documentation if they need to know the exact behaviour anyway.

That being said, you're right that it is odd having only a single loop* function. It sounds like we should just go with stop unless you all are fans of break_dispatch.

geky commented 8 years ago

Actually, thoughts on queue.break_dispatch() for breaking out of dispatch loops?

kjbracey commented 8 years ago

Yeah, break_dispatch is good.