Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#18363 closed enhancement (implemented)

Tor could use a publish/subscribe abstraction

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: modularity, tor-modularity, TorCoreTeam201605, TorCoreTeam-postponed-201604, review-group-1
Cc: Actual Points:
Parent ID: Points: 3
Reviewer: dgoulet Sponsor: SponsorS-can

Description

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.

Child Tickets

Change History (32)

comment:1 Changed 4 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 4 years ago by nickm

Type: defectenhancement

comment:3 Changed 4 years ago by isabela

Sponsor: SponsorSSponsorS-can

comment:4 Changed 4 years ago by nickm

Summary: Tor could use a publish/subscribe abstrctionTor could use a publish/subscribe abstraction

comment:5 Changed 4 years ago by nickm

Priority: MediumHigh

comment:6 Changed 4 years ago by nickm

Keywords: TorCoreTeam201604 added

For March, I aim to have alpha code here. For april I aim to merge.

comment:7 Changed 4 years ago by nickm

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

comment:8 Changed 4 years ago by nickm

Status: acceptedneeds_review

comment:9 Changed 4 years ago by dgoulet

Reviewer: dgoulet

comment:10 Changed 4 years ago by nickm

Keywords: tor-modularity added

comment:11 Changed 3 years ago by nickm

Keywords: TorCoreTeam201605 TorCoreTeam-postponed-201604 added; TorCoreTeam201604 removed

April is over; calling these april tickets postponed into may.

comment:12 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

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.

comment:13 in reply to:  12 ; Changed 3 years ago by cypherpunks

Replying to dgoulet:

  • I think a pubsub_free_ would be useful because right this leaks the static pubsub_topic_t name ## _topic_ = { NULL, 0 };.

dgoulet, dude!

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.

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 be DECLARE_*, definition is the same as implementation.)

comment:14 in reply to:  13 ; Changed 3 years ago by dgoulet

Replying to cypherpunks:

Replying to dgoulet:

  • I think a pubsub_free_ would be useful because right this leaks the static pubsub_topic_t name ## _topic_ = { NULL, 0 };.

dgoulet, dude!

Ahah... I really thought this one was a pointer. Nvm :)

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.

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 be DECLARE_*, definition is the same as implementation.)

Agree.

comment:15 in reply to:  14 Changed 3 years ago by cypherpunks

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_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?
Last edited 3 years ago by cypherpunks (previous) (diff)

comment:16 Changed 3 years ago by nickm

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.

comment:17 Changed 3 years ago by nickm

Oh, wrt the "quite a number of symbols declared outside of any macro" -- which symbols did you have in mind?

comment:18 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

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.

comment:19 in reply to:  18 Changed 3 years ago by cypherpunks

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.)

comment:20 Changed 3 years ago by 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.

Yeah, no.

Please spell it out for me.

s/DEFINE/DECLARE/g.

Ah, that one. Sounds reasonable.

comment:21 in reply to:  20 Changed 3 years ago by cypherpunks

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_keeporder moves the array content and reduces the length; _FOREACH is oblivious.
  • Call subscribe while inside a notification handler: _insert moves 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; _FOREACH is oblivious.

comment:22 Changed 3 years ago by nickm

Keywords: review-group-1 added

comment:23 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision
  • DECLARE/DEFINE change hasn't been changed so I'm assuming the current branch needs a revision.
  • 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'm currently happy with the latest commits addressing some issues. I'll let cypherpunks here maybe continue his/her review.

comment:24 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

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.

comment:25 Changed 3 years ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:26 Changed 3 years ago by nickm

Thanks David! Squashing & Merging. Calling the other stuff "can fix later if needed".

comment:27 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

comment:28 Changed 3 years ago by arma

util/pubsub/pubsub_basic: [forking] *** Error in `./src/test/test': free(): invalid next size (fast): 0xf7890ee0 ***

comment:29 in reply to:  28 ; Changed 3 years ago by cypherpunks

pubsub_subscriber_t *r = tor_malloc_zero(sizeof(r));

Nice.

comment:30 in reply to:  28 Changed 3 years ago by cypherpunks

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.

comment:31 in reply to:  29 Changed 3 years ago by cypherpunks

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.

comment:32 Changed 3 years ago by isabela

Points: medium3
Note: See TracTickets for help on using tickets.