bridge descriptor dormancy: Define an enum and semantics

This is part of #630 (closed).

I plan that there will be a new method on BridgeDescMgr set_dormancy, and a new argument to the constructor.

Splitting this off now to get feedback on two questions:

1. Is this semantics correct?
2. Should set_dormancy be part of BridgeDescProvider?

If the answer to Q2 is "yes", the type I'm adding in this MR needs to move down the stack.

Assigning @nickm as reviewer since as the author of the consuming code I think he knows the answer to these questions.

requested review from @nickm

assigned to @Diziet

Long-term observations

I think that in the long run we want to converge on TaskHandle, or something like TaskHandle, as a uniform way to manage dormancy. (Such a "something" doesn't need to do all the scheduling stuff, or have the exact same methods, as TaskHandle: it just needs to have the "suspend/resume/cancel" methods.)

In the long run:

• I don't want every crate to define its own own enum Dormant, and have to define its own set_dormancy method implemented in a slightly different way. But there's precedent for that
• I don't want the behavior of arti_client::client::tasks_monitor_dormant to grow ever more complex over time. I think that the call to chanmgr::set_dormancy in that function right now is an undesirable thing, and that instead I'd like to have that function just be the loop over TaskHandles.

#433 is somewhat related to this. If we were going to move everything into a crate, I'd suggest we start a new tor-tasks or tor-taskmgr or tor-scheduling crate or some such, and try to move the schedule code out of tor-rtcompat.

I would also suggest that we not try to refactor TaskHandle for %Arti 1.1.0: Anticensorship ready .

Short-term observations

I think that set_dormancy can remain as a method of BridgeDescMgr for now, to the extent that we don't believe this is a long-term API.

The semantics seem okay to me.

I think TaskHandle is rather the wrong shape for a general thing, because it is entangled with sleeping in a way that not every consumer wants. My intended implementation in BridgeDescMgr couldn't use TaskSchedule.

I agree that we don't want everyone to have their own enum Dormant. My preferred solution would be:

• Make a single enum Dormant in some lowish-level crate
• Plumb it explicitly into every place that needs it, probably by having them all take postage::watch::Receiver

Possible variations include:

• Bury the postage::watch::Receiver in a newtype of our own. I don't think this is worth it for a type we expose only below arti-client but I expect you will think it is worth it
• Have the SleepProvider fully handle the dormancy state, like it does now for TaskSchedule. So the SleepProvider would have a method to get the watch::Receiver (or our newtype). I'm not entirely enamoured of the way that TaskSchedule entangles "be runtime-agnostic" with "support becoming dormant" but I guess it's convenient and avoids additional plumbing.

I think TaskHandle is rather the wrong shape for a general thing, because it is entangled with sleeping in a way that not every consumer wants. My intended implementation in BridgeDescMgr couldn't use TaskSchedule.

Right. I think that we want TaskHandle to be disentangled from sleeping and from TaskSchedule.

But this is a long-run thing and maybe we should just table it for now.

Right. I think that we want TaskHandle to be disentangled from sleeping and from TaskSchedule

I think we're in agreement, enough to carry on, anyway. I'll continue with my current approach. This commit will be the base of the implementation MR then.

added 2 commits

• eeb6d526 - bridge descriptor dormancy: Add a TODO for this enum
• c6234428 - bridge descriptor dormancy: Accept the dormancy value

Compare with previous version

I have added the APIs to take the Dormancy but not wired it up either internally or externally. Those will be two separate MRs on top of this one.

mentioned in merge request !850 (merged)

mentioned in merge request !851 (merged)

This LGTM now, but the new Dormancy argument seems to require a BREAKING comment in semver. I wouldn't sweat it, since this API will have BREAKING changes from other stuff too. Please feel free to add (or not) as you see fit, then merge.

approved this merge request

merged

mentioned in commit 7efbc600

mentioned in issue #634