[tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction

[Tor Bug Tracker & Wiki]

#18363: Tor could use a publish/subscribe abstraction
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  nickm
     Type:  enhancement                          |         Status:
 Priority:  High                                 |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  modularity, tor-modularity,          |        Version:
  TorCoreTeam201605, TorCoreTeam-                |     Resolution:
  postponed-201604                               |  Actual Points:
Parent ID:                                       |         Points:  medium
 Reviewer:  dgoulet                              |        Sponsor:
                                                 |  SponsorS-can
-------------------------------------------------+-------------------------

Comment (by cypherpunks):

 Replying to [comment:14 dgoulet]:
 > Replying to [comment:13 cypherpunks]:
 > > It's supposed to be an inter-module facility, remember? Think about
 it.
 >
 > It is but that doesn't mean it has to be used inter-module _all_ the
 time thus controlling the linkage here is cheap and desirable imo.

 Sure, and I conceded as much in the my sentence. But my reminder was also
 directed at your item 2. Your feeling is wrong ;).

 I'll spell it out, now that I've read the whole thing.

 nickm's data structures seem fairly decoupled, I can see a few different
 possibilities.  Let's see if I understood it correctly. (You correct me
 nickm.)

 The more straightforward usage would be 1 publisher, n subscribers.
 {{{
 T-pubsub.h:
         #include "pubsub.h

         DECLARE_PUBSUB_TOPIC(T)

         [...]

 T-publishers-private.h:
         #include "T-pubsub.h"
         DECLARE_NOTIFY_PUBSUB_TOPIC(static, T)

         [...]

 T-publisher.c:
         #include "T-pubsub.h"
         #include "T-publisher-private.h"

         IMPLEMENT_PUBSUB_TOPIC(static, T)

         Use T_notify() and T_clear() here.  But maybe also T_subscribe()
 and
         T_unsubscribe().

         [...]

 any-T-subscriber.c:
         #include "T-publisher.h"

         Implement your T_subscriber_fn_t here, or maybe grab a generic one
 from
         somewhere else (maybe the publisher provides one).

         Use T_subscribe() and T_unsubscribe() here.
 }}}

 But ISTM that n publishers, n subscribers is also possible.
 {{{
 another-T-publisher.c:
         #include "T-pubsub.h"
         #include "T-publisher-private.h"

         // Note: no IMPLEMENT.

         Use T_notify() and T_clear() here.  But maybe also T_subscribe()
 and
         T_unsubscribe().

         [...]
 }}}

 The choice of where to put the definition for `T_event_data_t` and
 `T_subscriber_data_t` seems quite open, since the functions in "pubsub.c"
 only shuffle pointers around and never dereference them.  So it
 effectively depends on what the notification handler does with its
 arguments (plus maybe style and mental health).

 If the T-publisher(s) need to send actual data with the event (the event
 is more than just a "signal"), then, logically, T-publisher(s) and
 T-subscribers need to have the definition for `T_event_data_t` available.
 So it should probably go on the public "T-pubsub.h" header.

 If not, then I don't see why you couldn't always pass NULL.

 If there's a generic T-notification handler somewhere, then the definition
 for `T_subscriber_data_t` should probably be alongside.  If instead every
 T-subscriber implements his own T-notification handler then the definition
 can be in the c unit of each; hell, they could even be all different.

 ---

 Further review comments:

 - I don't think you use "tor_queue.h" included in "pubsub.h".

 - I don't like the `T_subscriber_fn_t` name (nor the "generic" type name
 nor the argument name).  A name that conveys "T notification handler", or
 similar, would be better IMHO.  But T_notification_handler_fn_t is a tiny
 bit unwieldy.

 - Why does `pubsub_subscriber_fn_t` take a single `void*` argument?  Why
 not `(void*, void*)`?  I see that it's going to be casted anyway, but is
 there any reason?

 - In `T_call_the_notify_fn_`, maybe document that the reason this function
 exists is so that the function pointer can be casted back to the
 appropriate type (noting that is not necessary to cast any of the 2 data
 pointers, in C).

 - `T_subscriber_t` is just used as a type tag, memory is never accessed as
 a `T_subscriber_t` (it can't: the type is never defined, only declared),
 instead it is always cast to and from `pubsub_subscriber_t` before and
 after accessing.  Correct?

 - In "pubsub.h", there are quite a number of symbols declared outside of
 any macro.  Why didn't they go inside the implementation macro?  Where
 else are they needed?

 - "Funny" that low priority means high priority ;).

 - Did you think about "interesting" interactions? E.g. unsubscribing from
 inside a notification handler: is your "smartlist" (more like resizable
 array) suitable for work-queue-type usages where you modify the queue as
 you traverse it?

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18363#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online