refactor(rpc): remove mutable callback#6786
Conversation
| requests/notifications. It's job is to initialize the session | ||
| state. *) | ||
| -> ?on_upgrade:('a Session.t -> Menu.t -> unit Fiber.t) | ||
| (** Called immediately after the client has finished negotitation. *) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also, will this callback be called exactly once?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Anyway, if you'd rather not change the name now, feel free to merge as is.
There was a problem hiding this comment.
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..)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I like the version with two callbacks, for success and failure. But as you say elsewhere, maybe we don't even need them.
|
Thanks for the heads up! It looks like we are not using |
instead of taking this upgrade callback is a mutable argument, we treat it as every callback and let it be provided in the [Handler] ps-id: fdab2a5e-bab6-4f34-af1c-b5ede91d525d Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
a6932d6 to
793bdde
Compare
|
Thinking about this again, I'm not even sure these callbacks are needed. Currently, they have two uses:
We can replace these callbacks with more direct features.
|
|
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. |
|
Agreed that not having callbacks seems cleaner. Also, some of the functionality can probably be exposed via ivars, e.g. |
* main: (148 commits) refactor(rpc): remove mutable callback (ocaml#6786) refactor(stdune): make [Id.t] immediate (ocaml#6795) refactor(melange): share mode handling (ocaml#6799) refactor(scheduler): remove duplicate types (ocaml#6785) refactor: move cram stanza definition (ocaml#6800) fix: correctly validate argument to top-module (ocaml#6796) refactor: move generate_sites_module to own module (ocaml#6798) fix(melange): check rules (ocaml#6789) refactor(rpc): make [Call.to_dyn] public (ocaml#6797) refactor(rpc): invalid session errors (ocaml#6787) refactor(melange): remove js globs (ocaml#6782) doc: fix version indication for "dune ocaml top-module" (ocaml#6792) refactor: print shutdown exception (ocaml#6784) refactor(rpc): add [Call.to_dyn] (ocaml#6790) fix: do not impose no_sandboxing on ocamldep (ocaml#6749) refactor(melange): move stanza definition (ocaml#6775) fix: handle missing CLICOLOR_FORCE correctly (ocaml#6781) Revert --display tui (ocaml#6780) fix: render pending messages refactor(coq): inline coqc_rule ...
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.