Repository navigation
Fix latent bugs found during the pre-release review - #907
Merged
Merged
Conversation
Mod_timeouts.set_global_ always passed Mod_sessadmin.update_serv_exp to set_timeout_, whatever the kind of timeout. Since the three setters were merged in 2015, changing the global data or persistent timeout with ~recompute_expdates:true updated the expiry dates of service states and left data and persistent states untouched. Dispatch on the kind, as the update_exp helper added at that time was meant to (it was removed as dead code in 2017).
set_default_global_persistent_data_state_timeout passed `Service to Mod_timeouts.set_default_global, so it changed the default timeout of service states and left persistent states unchanged.
With a `Session_group scope, set_max_service_states_for_group_or_subnet searched the group of groups in Mod_sessiongroups.Data and set_max_volatile_data_states_for_group_or_subnet searched it in Mod_sessiongroups.Serv. The two modules have separate tables, so the limit reached the group of the other kind of state, or nothing. Use Serv for service states and Data for data states.
close_service_state_if_empty decides whether a browser service session can be closed by counting its tab sessions, but it counted them in Mod_sessiongroups.Data, the table of tab data sessions. Count them in Mod_sessiongroups.Serv, as Mod_gc.service_session_gc does.
remove, up and set_max only act on the Dlist node, so they behave the same in Mod_sessiongroups.Serv and Mod_sessiongroups.Data. A few call sites used the module of the other kind of session, which made them look like the table mix-ups fixed in the previous commits. No behaviour change.
clean called Notif_hashtbl.remove from inside Notif_hashtbl.iter, whose behaviour is unspecified when the table is modified during the iteration. Use filter_map_inplace, which is designed for this.
notify ~notfor:(`Id id) tested the identity with the polymorphic equality, ignoring the equal_identity function the functor argument provides for that purpose (and which the weak table already uses). It could raise on identities containing closures, or still send the notification to an identity that equal_identity considers equal to id.
position built the (start, stop) pair of a client value or an injection from loc_start twice, so Lib.pos_to_string always printed a single character position in client-side error messages. Take stop from loc_end.
Pexp_fun and the argument type of Typ.alias belong to the AST that ppxlib selects (5.0 up to ppxlib 0.35, 5.2 from 0.36), not to the compiler, but they were guarded with ocaml_version. The PPX failed to build on OCaml 4.14 with ppxlib 0.38, and the reverse mismatch hits OCaml >= 5.3 with ppxlib 0.35. Guard them with ast_version instead, through a small typ_alias helper. The Otyp_* patterns come from the compiler and keep their ocaml_version guards; the two >= 5.1 Otyp_alias branches become one. Checked by building the PPX on OCaml 4.14.1/ppxlib 0.35, 4.14.2/0.38, 5.2.0/0.35, 5.3.0/0.37 and 5.4.0/0.38.
filter_na_get_params and relink_process_node compared String.sub s 0 n with a prefix without checking that s has at least n characters, so a shorter name raised Invalid_argument, unlike the sibling functions split_prefix_param and remove_prefixed_param. Neither case is reached today (the client never forces si_na_get_params, and process node ids are generated long), but the functions must not depend on that.
The server side of Shared.React.S.Lwt.merge_s dropped the accumulated synced flag and kept the one of the last signal, so a merge whose last signal was synced was reported as synced even when others were not. Combine the flags with &&, as FakeReact.S.merge and the l*_s functions do.
append_suffix is meant to replace the eliom_suffix_internal_name marker that ends the subpath of a service with suffix parameters, but the pattern [_eliom_suffix_internal_name] was a variable and matched any one-element list. Compare with Common.eliom_suffix_internal_name explicitly. preapply only calls it when the service takes a suffix, in which case the marker is always last, so nothing changes in practice.
Int_map ordered its keys with ( - ), which overflows when the keys are far apart (for instance max_int and a negative key) and then breaks the map invariants.
http_put and http_delete were copies of http_post and the client sent every call to a PUT or DELETE service as a POST request. Add an optional ~override_method parameter to send, passed to XmlHttpRequest.perform_raw_url for the first request only (full XHR redirections are followed with GET, as for POST), and use it in http_put and http_delete.
Html.Id.get_element and Svg.Id.get_element are documented to return None when no element has the given id, but they only caught the Failure raised for non-element nodes, while Client_core.getElementById raises Not_found for an unknown id. Catch both.
The parser of <omitpersistentstorage> checked its input with assert, so an attribute on the element, or a child other than a single-attribute <header/>, stopped the server with an Assert_failure. Match the expected shapes directly and raise Error_in_config_file otherwise, like the other Eliom options.
convert_attr turned the Invalid_argument raised by bool_of_string into Error_in_config_file, but it is also used with int_of_string, which raises Failure: <cacheglobaldata cache="x"/> stopped the server with an uncaught Failure "int_of_string". Catch both.
The four GC frequency options tested for "infinity" (meaning no garbage collection) only when float_of_string failed, but float_of_string accepts "infinity", so the value gave Some infinity and started a collector that sleeps forever. Test "infinity" first, in a parse_gc_frequency function shared by the four options.
balat
marked this pull request as ready for review
September 26, 2026 15:59
Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly, without protecting it, when the promise it watches is already rejected. The functions on persistent states could then raise an unexpected error synchronously instead of returning a rejected promise, for instance with a synchronous storage backend, which breaks the callers that iterate over states.
reset_unreadable is the handler of the Lwt.catch around the reads of persistent references. Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly when the read promise is already rejected, as it is with a synchronous storage backend: Reference.get then raised the storage error instead of returning a rejected promise.
Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly when the promise it watches is already rejected, so an immediate failure of the wait of a stateless request escaped synchronously from the service handler.
Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly when the promise it watches is already rejected, so an immediate failure of an XHR, a cancellation for instance, escaped synchronously from Request.send.
Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly when the promise it watches is already rejected, so an immediate failure of the call to the bus service escaped synchronously from the write function.
Lwt.reraise raises synchronously, and Lwt.catch runs its handler directly when the promise it watches is already rejected, so an immediate failure of typed_apply escaped synchronously.
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
1 participant
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.
Latent bugs found during a pre-release read of the whole code base. Each commit fixes one bug and adds a CHANGES entry when users can see the difference. The quality cleanups that do not change behaviour will come in a separate PR.
Sessions and timeouts (
State,Mod_timeouts,Mod_sessiongroups):~recompute_expdates:true, changing the global data or persistent timeout recomputed the expiry dates of service states instead. The bug dates from 2015: the dispatcher meant to choose the right updater was never called, and was later removed as dead code.set_default_global_persistent_data_state_timeoutset the default timeout of service states.set_max_{service,volatile_data}_states_for_group_or_subnetwith a session-group scope looked the group up in the table of the other kind of state.close_service_state_if_emptycounted tab data sessions when deciding whether a browser service session could be closed.Servfor service nodes andDatafor data nodes everywhere, so that the call sites no longer look like the mix-ups above.Notif:
cleanremoved entries from the hashtable while iterating over it.notify ~notfor:(Id id)used the polymorphic equality instead ofequal_identity`.PPX:
Pexp_funandTyp.aliaswere guarded withocaml_version, but their shape depends on the ppxlib AST (ast_version). The PPX did not build on OCaml 4.14 with ppxlib ≥ 0.36. It now builds on 4.14.1/0.35, 4.14.2/0.38, 5.2.0/0.35, 5.3.0/0.37 and 5.4.0/0.38.loc_start, so error messages showed a single character.Client:
Request.http_putandhttp_deletesent POST requests. The server routes on the real method, so client calls to PUT and DELETE services could not reach them. The fix adds?override_methodtoRequest.send.Html.Id.get_elementandSvg.Id.get_elementraisedNot_foundfor an unknown id instead of returningNoneas documented.Lwt:
Lwt.catchandLwt.try_bindre-raised withLwt.reraise, which raises synchronously. When the promise it watches is already rejected,Lwt.catchruns its handler directly, without protecting it, so these functions raised the exception instead of returning a rejected promise: theStatefunctions on persistent states, reading a persistentReference(errors are immediate with a synchronous storage backend), stateless Comet requests,Request.send,Buswrites and the clienttyped_apply. They now useLwt.fail. The same pattern, introduced by mistake inRoute_basewhile preparing Pre-release quality pass #908, made the lookup of a tab-session Comet service of Ocsigen Start fail, so the page reloaded in a loop; this is how it was found.Smaller fixes:
Shared.React.S.Lwt.merge_stook thesyncedflag of the last signal only.Int_mapcompared its keys with( - ), which overflows.<omitpersistentstorage>config stopped the server withAssert_failure.cacheattribute of<cacheglobaldata>stopped the server with an uncaughtFailure.infinityof the GC frequency options did not disable the collection, sincefloat_of_stringaccepts it.append_suffixused a pattern variable where a comparison with the suffix marker was meant (no change in practice).Every commit builds (
dune build, OCaml 5.4).dune build @runtestpasses.