Suggest #5847 use singleton pattern

[erights]

@warner , #5847 provides the perfect example for showing off the comparative simplicity of the singleton pattern.

@Chris-Hibbert the before-and-after here may be useful for the durability class.

@warner if you like this, feel free to merge it into your PR without need for further coordination. I mean this PR only as a review suggestion on #5847

Reviewers. be sure to "Hide whitespace" differences in github when reviewing.

[erights]

Notice this line. One consequence of using the singleton pattern for entangled facets, as here, is that there are a lot more use-before-define, to the point that being warned about them is just a noisy distraction.

[warner]

Thanks, I'll take a look. When I started looking into vivifySingleton (for the single-facet Kinds, at least), one counter-pressure I realized was a desire to have all the Kind definitions for the file appear in a single block at the beginning, as a form of maintenance reminder, so that future editors could easily see what Kinds their upgrade had an obligation to support. There are ways to retain that pattern for simple variable definitions (if you're willing to use let instead of const and maybe add a a few !== undefined assertions), but I don't see a similar thing for vivifySingleton. If the code in question only defines one, the issue probably isn't so visible.

I may decide to just use a comment at the top, which calls out a list of the Kind obligations, in which case I can switch to vivifySingletons elsewhere.

[warner]

A non-obvious property of my original version is that you could call createTimerService() multiple times, because the only thing it's doing is to update the let timerDevice and returning the timerService. So in a pinch, we could perform some sort of kernel-level upgrade/bugfix that replaced the timer device, but didn't actually upgrade the timer vat (e.g. if it turns out we didn't capture enough data to perform a successful upgrade somehow).

vivifySingleton must be called exactly once per vat version: if you call it twice you'll be re-defining the Kind, if you call it zero times (in a non-initial version) you'll be failing to define it in that version. So calling it from an externally-visible API imposes that exactly-once restriction on the external caller too.

I think I'm going to apply this, but with one change. I think I'll move most of the code back into buildRootObject rather than inside createTimerService, including the exactly-once timerService = vivifySingleton() call. Then the externally-visible createTimerService can go back to its original version, allowing it to be called multiple times. (It should really be named registerTimerDeviceAndReturnTimerService, or split into registerTimerDevice and getTimerService, but the old name is in use among a lot of clients right now, so I'll defer a potential rename/refactor for another day).

[erights]

Thanks, I'll take a look. When I started looking into vivifySingleton (for the single-facet Kinds, at least), one counter-pressure I realized was a desire to have all the Kind definitions for the file appear in a single block at the beginning, as a form of maintenance reminder, so that future editors could easily see what Kinds their upgrade had an obligation to support. There are ways to retain that pattern for simple variable definitions (if you're willing to use let instead of const and maybe add a a few !== undefined assertions), but I don't see a similar thing for vivifySingleton. If the code in question only defines one, the issue probably isn't so visible.

I may decide to just use a comment at the top, which calls out a list of the Kind obligations, in which case I can switch to vivifySingletons elsewhere.

Good discussion. Since the successor code must revive all kinds, the successor author is gonna grep for all of them anyway. A comment at the top is a fine idea. What we (@Chris-Hibbert and I have been doing is to rename the enclosing abstraction, createTimerService in this case, to vivifyTimerService and to give it the baggage as its first parameter. If it is named vivifySomething it must vivify something, which is also a good clue.

An additional reason for the rename is to signal that the first-crank constraint applies to it as well. Anything it is re-vivifying from the previous incarnation it must now do in the first crank. So this vivifyTimerService itself must be called in the first crank for those.

[erights]

(if you're willing to use let instead of const and maybe add a a few !== undefined assertions)

The const vs let is much of the point! With the let, if something messes up and deferences the variable before it is initialized, bad things happen, unless you explicitly put in checks like

      assert(wakeupHandler, 'reschedule() without wakeupHandler');

With const you know that it the variable is dereferenced before it is initialized---the temporal dead zone---that the language will throw the error. I left the line above in this delta-PR but I probably should have removed it. I'll leave that to you after merging into the base PR ;)

Even better, as a reviewer, when I see a let I immediately wonder "Is this ever assigned to aside from assignment which serves as an initialization?" Worrying about state changing out from under me is one of my first worries as a reviewer. Yes, even with a let I can answer quickly by grep. But it still makes the code less understandable.

Btw, our first effort, the ertp-service, went through exactly the same evolution. When I first coded it with defineDurableKind* I had a tangle of lets and late initialization. I just accepted it as a necessary price.

[erights]

A non-obvious property of my original version is that you could call createTimerService() multiple times, because the only thing it's doing is to update the let timerDevice and returning the timerService. So in a pinch, we could perform some sort of kernel-level upgrade/bugfix that replaced the timer device, but didn't actually upgrade the timer vat (e.g. if it turns out we didn't capture enough data to perform a successful upgrade somehow).

OMG, I had to read this several times to get it. That is interesting! I believe it is compatible with the change to vivifySingleton, and certainly requires that timerDevice remain a let. Now that you've explained it here and I finally understand it, I see that your comment in the code does explain this too. I completely missed it and would not have thought of it.

I think I'm going to apply this, but with one change. I think I'll move most of the code back into buildRootObject rather than inside createTimerService, including the exactly-once timerService = vivifySingleton() call. Then the externally-visible createTimerService can go back to its original version, allowing it to be called multiple times. (It should really be named registerTimerDeviceAndReturnTimerService, or split into registerTimerDevice and getTimerService, but the old name is in use among a lot of clients right now, so I'll defer a potential rename/refactor for another day).

Yes. Now that I understand, that is exactly the right thing to do. And never mind that at https://github.com/Agoric/agoric-sdk/pull/5862#issuecomment-1201955257 I used the example of createTimerService vs vivifyTimerService. It was the wrong example, but I think my point remains valid.

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #5847. Mergify cannot evaluate rules on this PR. ⚠️