Tor could use a publish/subscribe abstraction
Many places in our codebase, we have a function "foo_has_occurred()" that is called whenever foo happens, and which dispatches into many other semi-unrelated modules. These functions tend to be fairly huge modularity violations, and create many surprising edges in our module callgraph. Using a publish/subscribe pattern would be the usual way to enforce these divisions.
Activity
changed milestone to %Tor: 0.2.9.x-final
The branch 'pubsub' in my repository has a generic implementation that could easily become more complex as needed down the road.
The unit tests need a little more work; right now, they're just at the "make sure it doesn't crash" level.
There are extra flags on several functions for extensibility.
Future improvements might include:
- Multithreading
- Asynchronous notify
- Explicit acknowledgement
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_, seemsn_okis 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, notn_badwhich is what the comment suggests. -
I think a
pubsub_free_would be useful because right this leaks thestatic 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) == 429496minutes 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 touint64_t.
unsigned n_events_fired;- Do we want
pubsub_clear_to resetn_events_fired?
== Thoughts:
- Could it be possible to use void data pointer instead of
_subscriber_data_tand_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 aconnectionobject 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)-
Why is
DEFINE_PUBSUB_TOPICandDEFINE_NOTIFY_PUBSUB_TOPICare separated? I feel like not having a notify function defined will end up breakingIMPLEMENT_PUBSUB_TOPICso maybe merge them together? -
Is there a reason why only the
notifyandclearcan have a different linkage? I can see myself wanting all the functionsstatic. -
One part that will be annoying is the
foobar_subscriber_tobject thatpubsub_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" likefoobar_unsubscribe_all()which brings a clearer semantic imo.
Trac:
Status: needs_review to needs_revision- Seems to be an extra space typo in
Replying to dgoulet:
- I think a
pubsub_free_would be useful because right this leaks thestatic pubsub_topic_t name ## _topic_ = { NULL, 0 };.
dgoulet, dude!
-
Why is
DEFINE_PUBSUB_TOPICandDEFINE_NOTIFY_PUBSUB_TOPICare separated? I feel like not having a notify function defined will end up breakingIMPLEMENT_PUBSUB_TOPICso maybe merge them together? -
Is there a reason why only the
notifyandclearcan have a different linkage? I can see myself wanting all the functionsstatic.
It's supposed to be an inter-module facility, remember? Think about it.
(Though, about the linkage: it may actually be useful to have the option anyway.)
(Also:
DEFINE_*should really beDECLARE_*, definition is the same as implementation.)- I think a
-
Replying to cypherpunks:
Replying to dgoulet:
- I think a
pubsub_free_would be useful because right this leaks thestatic pubsub_topic_t name ## _topic_ = { NULL, 0 };.
dgoulet, dude! Ahah... I really thought this one was a pointer. Nvm :)
-
Why is
DEFINE_PUBSUB_TOPICandDEFINE_NOTIFY_PUBSUB_TOPICare separated? I feel like not having a notify function defined will end up breakingIMPLEMENT_PUBSUB_TOPICso maybe merge them together? -
Is there a reason why only the
notifyandclearcan have a different linkage? I can see myself wanting all the functionsstatic.
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.
(Though, about the linkage: it may actually be useful to have the option anyway.)
(Also:
DEFINE_*should really beDECLARE_*, definition is the same as implementation.)Agree.
- I think a
Replying to dgoulet:
Replying to 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 my next 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-publishers-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-pubsub.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-publishers-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_tandT_subscriber_data_tseems 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_tavailable. 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_tshould 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_tname (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_ttake a singlevoid*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_tis just used as a type tag, memory is never accessed as aT_subscriber_t(it can't: the type is never defined, only declared), instead it is always cast to and frompubsub_subscriber_tbefore 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?
-
-
I fixed some of the simpler issues. I'm going to wontfix the following ones:
- The static/public stuff. I think that I got it right, as cpunks describes above, but it'll be easy enough to fix later if we need it different.
- Function naming stuff; I don't mind the names. Also they're easy enough to change later if somebody makes a patch.
- Yes, foo_subscriber_t is only a tag.
- Low-valued priorities meaning "do this first" is all over POSIX; I'm not changing that.
That leaves these as my remaining items:
- Seeing if I can mess with the type definitions so that you don't need to declare a struct for non-compound message types.
- The "interesting" interactions as described above.
Replying to nickm:
Oh, wrt the "quite a number of symbols declared outside of any macro" -- which symbols did you have in mind?
Well, all of them! To wit:
- pubsub_subscriber_fn_t
- pubsub_subscriber_t
- pubsub_topic_t
- pubsub_subscribe_
- pubsub_unsubscribe_
- pubsub_clear_
- pubsub_notify_fn_t
- pubsub_notify_
These are implementation details, nobody should be using them. So, why even allow it? Why gratuitously polluting the namespace of every consumer of "pubsub.h"?
Replying to nickm:
Okay, both of the above issues should be 'resolved'. I've solved the concurrent-modification problem by just forbidding it for now; we can allow it later if needed.
Yeah, no. Not unless you change the data structure behind. I've looked a bit at the smrtlist functions/macros now, and indeed they are just relocatable arrays, so that's certainly not going to fly against reentrant calls (not something easy to avoid once you have this convenient callback machinery; call trees can grow complicated pretty quickly).
(PS: You said nothing about s/DEFINE/DECLARE/g.)
-
Why gratuitously polluting the namespace of every consumer of "pubsub.h"?
The implementation macros use these functions. The alternative would be to rename them and stick them in the macros. That didn't seem to have much point IMO.
Yeah, no.
Please spell it out for me.
s/DEFINE/DECLARE/g.
Ah, that one. Sounds reasonable.
Replying to nickm:
Why gratuitously polluting the namespace of every consumer of "pubsub.h"?
The implementation macros use these functions. The alternative would be to rename them and stick them in the macros. That didn't seem to have much point IMO.
Errh. Why would you need to rename them? AFAICS, you could simply put them all at the beginning of the IMPLEMENT macro, for example.
In "pubsub.c" you only need the types. It would be great if it were possible to put them in a header and include this from both "pubsub.c" and the IMPLEMENT macro, but that doesn't work, right? In any case, repetition can be avoided if the include inside the macro is scripted (a pre-preprocessing step).
Hmm. Well, at least the function prototypes can be easily hidden away in the macro.
Yeah, no.
Please spell it out for me.
Oh, I think I quoted too much and confused you. I actually think it's fine now with the "locked" guard. When I said "no" I was actually referring only to this part:
we can allow it later if needed.
And I qualified that: "unless...". And I believe you already agree with me in this, that's why you added the "locked" thing, right? But 2 examples anyway:
- Call unsubscribe while inside a notification handler:
_del_keepordermoves the array content and reduces the length;_FOREACHis oblivious. - Call subscribe while inside a notification handler:
_insertmoves the array content, relocates memory (this one is not be a problem in single-threading? there's one further indirection? not sure, will check again later), and increases the length;_FOREACHis oblivious.
- Call unsubscribe while inside a notification handler:
-
-
DECLARE/DEFINEchange hasn't been changed so I'm assuming the current branch needs a revision. -
Also, the whole need for a
subscriber_data_tstructure hasn't been addressed. I'm fine if you prefer it that way instead of making the caller choose whatever it want to pass.
I'm currently happy with the latest commits addressing some issues. I'll let cypherpunks here maybe continue his/her review.
Trac:
Status: needs_review to needs_revision -
-
DECLARE/DEFINE change hasn't been changed so I'm assuming the current branch needs a revision.
just did that in 8b75263e35ec5c217525fa67a019852b6b541ffc ; thanks for the reminder.
Also, the whole need for a subscriber_data_t structure hasn't been addressed. I'm fine if you prefer it that way instead of making the caller choose whatever it want to pass.
I think I did this already in ceadd497525120d0f77be7b5755ca5bc7015f326 though.
Trac:
Status: needs_revision to needs_review Replying to arma:
util/pubsub/pubsub_basic: [forking] *** Error in./src/test/test': free(): invalid next size (fast): 0xf7890ee0 ***` The issue has been reported in #19038 (moved).Replying to cypherpunks:
pubsub_subscriber_t *r = tor_malloc_zero(sizeof(r));Nice. This was indeed the cause and ticket:19038#comment:1 contains a patch based on your suggestion.
mentioned in issue #19012 (moved)
moved to tpo/core/tor#18363 (closed)
mentioned in issue tpo/core/tor#19012 (closed)
