Opened 12 months ago

Closed 7 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

Description

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 12 months ago by nickm

Status: assignedneeds_review

See branch messaging in my public repository, with PR at https://github.com/torproject/tor/pull/452

comment:2 Changed 12 months 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 12 months ago by dgoulet

Keywords: tor-pubsub added
Reviewer: catalyst

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

comment:4 Changed 11 months 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 11 months ago by nickm

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

comment:6 Changed 9 months ago by gaba

Keywords: s31-maybe added
Sponsor: Sponsor8

comment:7 Changed 9 months ago by gaba

Keywords: sponsor31-maybe added; s31-maybe removed

comment:8 Changed 9 months ago by nickm

I've made a new branch as messaging_v2, with a PR at https://github.com/torproject/tor/pull/648.

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 9 months ago by nickm

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

comment:10 Changed 9 months ago by nickm

Sponsor: Sponsor31-can

comment:11 Changed 9 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 9 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 8 months ago by gaba

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

comment:14 Changed 8 months ago by catalyst

Made some comments on github; still reviewing.

comment:15 Changed 7 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.
https://github.com/tlyu/tor/tree/bootpubsub

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 7 months ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 7 months ago by nickm

The branch is now squashed as messaging_v3, with a new PR for CI purposes at https://github.com/torproject/tor/pull/844

comment:18 Changed 7 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 7 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 7 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 https://github.com/torproject/tor/pull/859

comment:21 Changed 7 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 7 months ago by nickm

Actual Points: 18

comment:23 Changed 7 months ago by asn

Resolution: fixed
Status: merge_readyclosed

Merged! Hooray! Onwards!

Note: See TracTickets for help on using tickets.