Opened 2 years ago

Closed 19 months ago

#28226 closed enhancement (fixed)

Backend for an async publish-subcribe messaging system for cross-module communications

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: teor-merge, asn-merge, tor-pubsub, sponsor31-maybe, network-team-roadmap-2019-Q1Q2
Cc: Actual Points: 18
Parent ID: Points: 8
Reviewer: catalyst Sponsor: Sponsor31-must


In Mexico City I walked with Teor and Catalyst about a generic publish/subscribe system. Thanks for feedback from the network team, I have an initial API I like, and I've tried to do an implementation.

Child Tickets

Change History (23)

comment:1 Changed 2 years ago by nickm

Status: assignedneeds_review

See branch messaging in my public repository, with PR at

comment:2 Changed 2 years ago by nickm

Next steps for me here:

  • send the design document to tor-dev
  • Write the API for testing modules that use this
  • write the subsystem implementation.

comment:3 Changed 2 years ago by dgoulet

Keywords: tor-pubsub added
Reviewer: catalyst

(Declaring this new subsystem "tor-pubsub" :)

comment:4 Changed 2 years ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:5 Changed 2 years ago by nickm

I've pushed a few more commits based on your comments, to try to improve naming and documentation.

comment:6 Changed 21 months ago by gaba

Keywords: s31-maybe added
Sponsor: Sponsor8

comment:7 Changed 21 months ago by gaba

Keywords: sponsor31-maybe added; s31-maybe removed

comment:8 Changed 21 months ago by nickm

I've made a new branch as messaging_v2, with a PR at

It refactors the old branch considerably, and incorporates a bunch of your suggestions.

(It does not switch from using "auxdata" to using a subtype of "msg_t" for additional data as we had suggested, since I am not sure about that yet. We can do that part later if need be.)

comment:9 Changed 21 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.4.1.x-final

comment:10 Changed 21 months ago by nickm

Sponsor: Sponsor31-can

comment:11 Changed 21 months ago by nickm

Note from Catalyst: In "Add MESG as a new log domain.", N_LOGGING_DOMAINS needs to be incremented.

comment:12 Changed 21 months ago by catalyst

Partial review so far, by commit:

b7358da11 namemap
seems ok. I have a few misgivings about the type punning used for the temporary object, but it might be safe
d26a213c9 LD_MESG
seems ok, except for needing to bump the total as mentioned above
749456940 semicolon eater
seems ok. Maybe we want to uses this in ht.h, but that means changing all its users, so maybe not in this ticket
33c20c70d smartlist_grow
seems ok, assuming the sizes bounds check in smartlist_ensure_capacity() is correct, which from a quick glance it seems to be

comment:13 Changed 21 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added
Points: 8
Sponsor: Sponsor31-canSponsor31-must

comment:14 Changed 20 months ago by catalyst

Made some comments on github; still reviewing.

comment:15 Changed 20 months ago by catalyst

Status: needs_reviewneeds_revision

I wrote a proof of concept hooking up part of the new bootstrap reporting code to pubsub.

It builds but doesn't pass tests, because I haven't yet figured out how to hook up the pubsub mainloop stuff.

I commented in the pull request about most of difficulties I experienced with the code and consuming-developer documentation. There are a few more, but I think they're minor documentation issues.

I have some comments on architecture and design that it might not be worth delaying integration to address. I'll write them up soon.

How much work is it to finish hooking up pubsub to the mainloop and initialization code? Would you be willing to add commits to do that? It would be great to be able to test my POC. Thanks.

comment:16 Changed 19 months ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 19 months ago by nickm

The branch is now squashed as messaging_v3, with a new PR for CI purposes at

comment:18 Changed 19 months ago by catalyst

Status: needs_reviewmerge_ready

Rebased and checked my bootpubsub branch against the new pull request; seems good!

Right now I think these changes are good enough to merge. I still have comments about technical debt and long-term risks, but I'll put those in a separate ticket. (Some of these overlap with stuff Nick has sent me in private feedback.) It's probably better to clean those up later now that we have a working proof of concept of a consumer of this API.

comment:19 Changed 19 months ago by catalyst

Status: merge_readyneeds_revision

Nick said he would fix the log messages to stop logging numbers with names. He'll flip this ticket back to merge_ready when that's done.

comment:20 Changed 19 months ago by nickm

Status: needs_revisionmerge_ready

Done in messaging_v3.

I've made a new branch, messaging_v3_merged, to handle the (small) merge conflicts in master. PR at

comment:21 Changed 19 months ago by teor

Keywords: teor-merge asn-merge added

I'll merge to master if CI finishes today, otherwise asn can merge.

Please remember to update actual points!

comment:22 Changed 19 months ago by nickm

Actual Points: 18

comment:23 Changed 19 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged! Hooray! Onwards!

Note: See TracTickets for help on using tickets.