refactor(rpc): remove mutable callback

[rgrinberg]

instead of taking this upgrade callback is a mutable argument, we treat
it as every callback and let it be provided in the [Handler].

@snowleopard if you're using this internally, let me know if there are issues porting. I expect this to be straightforward.

[snowleopard]

The name is a little unclear to me. Shouldn't it be called on_receiving_menu or something like this? It's not like anything got "upgraded" here.

[snowleopard]

Also, will this callback be called exactly once?

[rgrinberg]

Yes, I don't like the name either. I just chose to preserve what was already there.

Also, will this callback be called exactly once?

At most once in fact. If the version negotiation fails, it will not be called.

Shouldn't it be called on_receiving_menu or something like this? It's not like anything got "upgraded" here.

It's a bit more than that because this is the earliest point where we actually send/receive requests/notifications. Or put another way, it's called immediately after version negotiation.

[snowleopard]

Maybe we could call it on_version_negotiation and make the argument option-like to indicate the failure of the negotiation? Then we can say it's called exactly once.

[snowleopard]

Anyway, if you'd rather not change the name now, feel free to merge as is.

[rgrinberg]

Maybe we could call it on_version_negotiation and make the argument option-like to indicate the failure of the negotiation? Then we can say it's called exactly once.

If negotiation fails then the session is dropped so there's not much we can do. So the signature would have to be:

; on_version_negotiation : [`Success of 'a Session.t * Menu.t | `Failure ] -> unit Fiber.t

Which is equivalent to:

; on_version_negotiation_success : 'a Session.t -> Menu.t -> unit Fiber.t
; on_version_negotiation_failure : unit -> unit Fiber.t

Personally, I find the latter to be more natural because that's how we handle the other callbacks already (on_init, on_terminate..)

[rgrinberg]

Then we can say it's called exactly once.

Or do you think this property is valuable enough that we should design our callbacks to satisfy it if possible?

Because right now on_terminate is also called "at most once" because it's not called for callbacks that failed to finish negotiation.

[snowleopard]

I like the version with two callbacks, for success and failure. But as you say elsewhere, maybe we don't even need them.

[snowleopard]

Thanks for the heads up! It looks like we are not using register_upgrade_callback.

[snowleopard]

Looks good!

[rgrinberg]

Thinking about this again, I'm not even sure these callbacks are needed. Currently, they have two uses:

1. Saving the client menu to display it when getting the status of all connected clients.
2. Sending requests to a client upon initialization inside a test.

We can replace these callbacks with more direct features.

1. Always save the Menu.t for all clients ourselves. It's tiny and I don't see the downside.
2. Add a function val version_negotitation_completed : 'a Session.t -> unit Fiber.t that the user can wait for.

[rgrinberg]

This PR is blocking some more important work but is still fairly minor so I'm going to merge it.

I'll open a PR in a parallel that get rids of these callbacks entirely in favor of the sugestions I've made above.

[snowleopard]

Agreed that not having callbacks seems cleaner. Also, some of the functionality can probably be exposed via ivars, e.g. (Session.t * Menu.t) option Ivar.t that would be filled after the negotiation (or some version of it instead of option).