[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #18363 [Core Tor/Tor]: Tor could use a publish/subscribe abstraction
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs