client: Direct-style API for Eliom_comet and Eliom_bus#844
Open
Julow wants to merge 10 commits intoocsigen:masterfrom
Open
client: Direct-style API for Eliom_comet and Eliom_bus#844Julow wants to merge 10 commits intoocsigen:masterfrom
Eliom_comet and Eliom_bus#844Julow wants to merge 10 commits intoocsigen:masterfrom
Conversation
`Lwt_stream` is difficult to migrate to other concurrency libraries because of its vast API and its many usecases. Its usage in `Eliom_comet` can easily be changed to a callback-based approach. This might increase performances as well.
Add a new callback-based API, `Eliom_bus.register`, which avoids using `Lwt_stream` internally. `Eliom_bus.stream` is re-implemented against the register API. `Eliom_bus.original_stream` is not re-implemented yet.
`Eliom_comet.register` and `Eliom_bus.register` propagate errors from the server using an `option` type. `Eliom_bus` ensures that the callback won't be called again after being called with `None`. This "end of stream or error" signal was present before through `Lwt_stream`, which could propagate the `Closed` exception. In both `Eliom_comet` and `Eliom_bus`, the callbacks are released when the channel closes to allow memory to be collected.
Implement the callback-based API that can be used with values of type [Eliom_comet.Channel.t] that can be passed from the server.
This simplifies the handling of messages in `Eliom_bus` by registering all callbacks directly into the underlying `Eliom_comet`. `Eliom_comet.register_wrapped` is removed in favor of `Eliom_comet.register` in the previous API. This changes requires signaling when the channel should be aware from `Eliom_bus`.
With the same intention as the previous commit, this simplifies the implementation of `Eliom_bus`. This removes the internal function `Eliom_comet.close` in favor of `Eliom_comet.Channel.close`, which is much easier to implement.
Callbacks are no longer an ever-growing list of functions. The changes are: - Callbacks are grouped by channel ID in a Hashtbl. This dramatically reduces the number of callback that need to be considered. - The channel position is stored along side the callback instead of in a closure. This will make writing a `unregister` function easier. - `hd_error_callbacks` is removed. This was needed only to workaround the internal complexity. Some code is moved for ordering. The `position` type and related code are wrapped in a `Position` module to improve readability.
Add `Eliom_comet.Channel.unregister` and `Eliom_bus.unregister` to unregister callbacks.
Contributor
Author
|
In the last 4 commits, I improve the code a lot and added the unregistration API. The test-case is uptodate: Julow/ocsigen-tictactoe#1 |
balat
reviewed
Jul 23, 2025
src/lib/eliom_comet.client.mli
Outdated
| No request will be sent. *) | ||
|
|
||
| val close : 'a t -> unit | ||
| (** [close c] closes the channel c. This function should be only use |
Contributor
Author
There was a problem hiding this comment.
I don't think this docstring is true anymore, there is no way to close the channel without also using Eliom_bus.
I made the close function public and fixed a potential memory leak in my last commit.
There were no way to close a channel when using `Eliom_comet` alone. This exposes the `close` function, which was internally used by `Eliom_bus`. It's also changed to make sure that registered callbacks are unregistered to avoid memory leaks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This changes the client-side API of
Eliom_cometandEliom_busto be more compatible with direct-style programming:Add a new callback-based API,
Eliom_bus.register, which avoids usingLwt_streaminternally.Eliom_bus.streamis re-implemented against the register API.Eliom_bus.original_streamis not re-implemented yet.Eliom_comet.Channelis an abstract type instead of aLwt_stream.Lwt_streamis difficult to migrate to other concurrency libraries because of its vast API and its many usecases.It's also callback-based but it's made to be easy to construct a
Lwt_streamon top of it.This might increase performances as well.
This was successfully tested against a small application that uses these APIs: Julow/ocsigen-tictactoe#1
Some work remains to be done:
Lwt.cancelorLwt.pick.Eliom_bus. For example, the callback list could possibly be removed and implemented directly againstEliom_comet.register. This change also opens the door for performance improvements and code simplifications inEliom_comet.Eliom_bus.original_stream. A comment inEliom_cometsays that messages start receiving only after the load phase, which makes it seems like it is exactly equal toEliom_bus.stream. It seems that the new API have the same behavior during the load phase and will not lose more messages than the old one when registering callbacks outside of the load phase. This remains to be checked.