Skip to content
This repository was archived by the owner on Aug 19, 2021. It is now read-only.

Remove the EventLoop class #14

Merged
merged 1 commit into from
Aug 10, 2016
Merged

Remove the EventLoop class #14

merged 1 commit into from
Aug 10, 2016

Conversation

geky
Copy link
Contributor

@geky geky commented Aug 9, 2016

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 Remove problematic templated overloads on callback-based functions mbed-os#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-
Copy link
Member

pan- commented Aug 9, 2016

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

* Already pending events may finish executing, but the queue will not
* continue to loop indefinitely.
*/
void break_();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename this member function and clarify what is going on when invoked ?

@geky
Copy link
Contributor Author

geky commented Aug 9, 2016

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

@geky geky force-pushed the remove-eventloop branch from 65ac99b to 16a1aa3 Compare August 9, 2016 09:22
@pan-
Copy link
Member

pan- commented Aug 9, 2016

@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
Copy link

kjbracey commented Aug 9, 2016

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

@kjbracey
Copy link

kjbracey commented Aug 9, 2016

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
Copy link
Contributor Author

geky commented Aug 9, 2016

@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
Copy link

kjbracey commented Aug 9, 2016

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
Copy link
Contributor Author

geky commented Aug 9, 2016

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 geky changed the title Removed the EventLoop class Remove the EventLoop class Aug 9, 2016
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.

- Split EventQueue::dispatch into overloaded functions
- Remove problematic template overloads in Thread constructor
- Exposed break_dispatch
- Renamed get_tick to tick to match the underlying C api
- Renamed DEFAULT_QUEUE_SIZE to EVENTS_QUEUE_SIZE to avoid name
  possible name collisions
@geky geky force-pushed the remove-eventloop branch from 16a1aa3 to 05b532d Compare August 9, 2016 22:11
@geky
Copy link
Contributor Author

geky commented Aug 9, 2016

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

@kjbracey
Copy link

Yeah, break_dispatch is good.

@geky geky merged commit 3338ca1 into master Aug 10, 2016
@geky geky deleted the remove-eventloop branch August 10, 2016 15:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants