[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
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 Some code review and then after some thoughts. Btw, this is neat, it's the
 kind of thing I would use once merged!

 * Seems to be an extra space typo in `pubsub_notify_`
 {{{
   ++ topic->n_events_fired;
 }}}

 * In `pubsub_notify_`, seems `n_ok` is basically useless counter there.

 * In `pubsub_notify_`, the comment says: `Return the number of nonzero
 return values`. But it returns 1 or 0 depending on if we a bad one
 happened, not `n_bad` which is what the comment suggests.

 * I think a `pubsub_free_` would be useful because right this leaks the
 `static pubsub_topic_t name ## _topic_ = { NULL, 0 };`. We should be able
 to fully clear the memory linked to a unused pubsub.

 * Why are we keeping "notify flags" if they are not used at all. Do you
 plan having some or just "in case"?

 * Imagine tens of thousands time a topic being notified every 60 seconds.
 I worry that this will overflow regularly. So `(2^32 / 10000) == 429496`
 minutes which is 7158 hours and thus after running for 298 days, wrap
 time. Ok, pretty large but still that could have undesired consequences in
 the future if unnoticed. Pretty cheap to have it set to `uint64_t`.
 {{{
 unsigned n_events_fired;
 }}}

 * Do we want `pubsub_clear_` to reset `n_events_fired` ?

 == Thoughts:

 1) Could it be possible to use void data pointer instead of
 `_subscriber_data_t` and `_event_data_t`. Not sure it's really good to
 force every user to create "data" wrappers. If they need one because for
 instance they need a lot of information being passed along, the caller
 should define one by itself. I suspect most of the time we'll pass back a
 `connection` object or some single data structure. So we could do
 something like this instead of more data structure which seems more simple
 and telling:

 {{{
 dir_connection_t new_dir_conn;
 [...]
 dirconn_open_subscribe(dir_conn_has_opened, NULL, 0, 0)
 [...]
 dirconn_open_notify(new_dir_conn)
 }}}

 2) Why is `DEFINE_PUBSUB_TOPIC` and `DEFINE_NOTIFY_PUBSUB_TOPIC` are
 separated? I feel like _not_ having a notify function defined will end up
 breaking `IMPLEMENT_PUBSUB_TOPIC` so maybe merge them together?

 3) Is there a reason why only the `notify` and `clear` can have a
 different linkage? I can see myself wanting _all_ the functions `static`.

 4) One part that will be annoying is the `foobar_subscriber_t` object that
 `pubsub_subscribe_()` returns. A user would have to keep them somewhere
 and index them using function pointer/data pointer pair. I wonder how we
 could make this easier. At least, `pubsub_clear_` will be useful for just
 cleaning everything instead of using all subscriber object. What about an
 "alias" like `foobar_unsubscribe_all()` which brings a clearer semantic
 imo.

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