Conversation
Core's components expose database and config access with no stated thread
contract, while every Client method carefully dispatches onto Core's loop.
Nothing distinguishes the two halves: `client.set_display_name(name, cb)` is
correct and `client.core.configs.user_profile().set_name(name)` compiles just
as cleanly and is a data race. The header comment on `globals` invites the
second outright ("can also be used by the application to persist settings").
The database itself is not the problem -- session-sqlite hands each thread its
own connection and says to use it that way. What is unsynchronised is
everything a component holds beside its tables: `_adopt_seed` rewrites a
std::string and a secure_buffer that the loop reads while polling, and the
config objects are built lazily and then mutated by `merge()`. There is not an
atomic or a mutex anywhere in globals, configs or devices.
So `CoreComponent` grows `on_loop()`, and every method that touches component
state asserts it. `on_loop()` is also true during construction, since a
component's `init()` necessarily runs on the constructing thread and no other
thread can have reached it yet.
Where an application legitimately calls one of these from its own thread, the
method now comes in the two forms Client already uses -- a `failable_function`
and a `block_t` -- and nothing else. Not three: `call_get` runs the job inline
when it is already the loop thread, so code already there uses the blocking
form and pays nothing. That covers `create_account`, `restore_account`,
`device_info`, `update_info` and `build_link_request`. `Globals`' get/set/erase
stay as they are and say why: one self-contained query each, touching nothing
cached.
`failable_function` and `block_t` move to <session/handler.hpp> so Core can use
the same convention; `session::client` re-exports them, so nothing that names
them changes.
The assertion immediately found a live bug, fixed here too because the suite
does not pass without it. `Network` builds its own `quic::Loop`, so a
`send_request` completion handler runs on the network's thread -- and
`Core::_send_poll` called `_handle_poll_response` straight from one, merging
configs and flushing their dumps off Core's loop on every single poll. It is
now marshalled, and `TestHelper::drain` lets the tests that drive a response by
hand wait for it the way production does.
Two related lifetime fixes fall out of the same reasoning. Core gains a
`JobQueue` of its own, declared last so it stops -- cancelling outstanding
component work -- before the components those jobs reach are destroyed, and
while `_loop` is still alive to process the stop. And Client's `_jq` existed
to cancel its deferred work on destruction but carried exactly one job:
`_async` and nine other sites deferred onto the loop's own queue instead, which
is not emptied until `~Loop`, the last thing `~Core` does. Every one of those
jobs holds `this` and reaches through it into Core's members, so they were
being drained throughout the destruction of every one of them. They now go on
`_jq`; `call_get` stays on the loop, since its caller is blocked inside it and
cannot have gone away.
Deferring work correctly meant remembering to reach for `_jq` rather than `loop`, and the two read identically at the call site -- which is why nine Client sites and `_async` itself got it wrong. A comment on `_jq` does not fix that; removing the alternative does. `Client::loop` is gone. Everything it was used for now has a wrapper, so the mistake is no longer writable inside Client, and the compiler found every site rather than leaving it to review. Anything that genuinely wants the loop still says `core.loop()`, which stays public and now documents what it costs: a job on the loop's own queue is not discarded until `~Loop`, the last thing `~Core` does. The wrappers are private on Client, whose deferring is all internal, and public on Core, where `loop()` was already reachable and this is the safer spelling of the same thing. `call_get` returns by value rather than perfectly forwarding. A reference handed back through it has outlived the job that produced it, which is the hazard the queue exists to close, so decaying it is the point rather than a limitation.
…them `test_core_configs.cpp` reached into the configs from the test thread, which is what the new assertion is about: a test is an application like any other, and the configs belong to Core's loop. `TestHelper::on_loop` wraps a block rather than a call, since a test case does several config operations in a row and they all want the same excursion. Three places could not simply be wrapped whole, and say so where they are: `reopen()` destroys the Core and so the very loop a wrapper would be running on, and Catch2's GENERATE and SECTION have to stay at test scope because the case is re-run for each. `test_client/`'s shared `*_from_another_device` helpers get the same treatment. The remaining direct reads in `test_client/` are not converted yet: several return a `string_view` into the config, which would outlive the excursion, so they need looking at one at a time rather than wrapping.
Two things the poll fix left behind, both the same shape as it. A config push's completion runs on the Network's own loop -- Network builds its own quic::Loop -- and clears `_push_in_flight`, confirms the pushed configs and dumps them, all of which is Configs' state and none of which is safe off Core's loop. The body moves into `_handle_push_response` so the callback can be one line of marshalling, the way `Core::_handle_poll_response` already is, and `Pending` moves to the class with it. Note what does *not* change: the `_alive` canary stays, and there is now a comment saying why. The Network owns these callbacks, so they can outlive Core entirely, and a stopped queue cannot cancel something that was never queued -- the canary is what makes reaching `jq()` safe in the first place, and the queue takes over from there. And `_poll_ticker` was declared with the rest of the polling machinery, so it was destroyed *after* the components a firing poll reaches. It is declared last now, which destroys it first: no poll can be in flight by the time anything it touches is being torn down. Before `_jq` rather than after, so that a poll cannot try to queue its response onto a queue that has already stopped, which throws rather than being ignored.
The remaining reads of `core.configs` from `test_client/`, which is the other half of what the assertion is about: a test is an application like any other, and the configs belong to Core's loop. `in_configs` goes in `common.hpp` rather than `config_helpers.hpp`, since it is not about config reconciliation -- it is for any test reaching past `Client` to check what was written underneath. It hands back a value on purpose: several of these read `get_name()`, which returns a `string_view` into the config, and that is dangling the moment the excursion ends. Those copy inside the lambda. Three sites bound `auto& contacts = c->core.configs.contacts()` and used it across several statements. A reference is exactly what cannot leave the loop, so they read what they need each time instead. `TestHelper::sync_contact` and `sync_convo_volatile` hop for themselves rather than making every caller do it: what they reach is a `_`-form on Client, which is only ever called from inside `_async` in the real thing. The `merge_*` helpers wrap their `receive_messages` for the same reason -- a real poll's merge arrives on the loop. Three tests were already doing this hop by hand with `loop().call_get`, which is where the idea came from; one of those is now spelled the same way as the rest.
_fail_connection moved the listeners into a local, erased the map entry, and then iterated the erased entry: a dereference of an invalidated iterator into a moved-from vector. No listener ever fired.
set_network could always be handed a second Network, and nothing about doing so worked: it stops and starts the libevent poll ticker off the loop thread, and ~Network fails the requests its router and transport are holding, which runs Core's poll continuation against a router that has just been destroyed. Throw instead, and record on the declaration what a real replacement has to do first. Also note why the snode bootstrap fetcher bypasses the router, and that session-router mode need not.
A QUIC handshake through a Session Router tunnel was budgeted with the figure chosen for a direct connect to a node's own address: 3s to cross a multi-hop path, after which the transport concludes the storage node is unreachable and strikes it in the SnodePool -- for the latency of a connection nominally made to ::1. Split the two. Direct handshakes go to 5s; a request that the router rewrote to the local end of a tunnel is marked as such and gets its own 10s. The transport cannot infer this from the address, which is loopback either way, so the request carries it. Also read the request's category before it is moved into the pending queue rather than after, and correct two option doc comments that named defaults the code had long since changed.
Nothing could reach us except as the response to something we sent: the transport's only inbound path was a request's own callback, and the stream it sends on had no handler registered for anything arriving the other way. A swarm subscription is delivered exactly that way, as a request of the storage server's own making, so it had nowhere to land. Register a generic handler on the connection's stream and pass what arrives up through Network, naming the node by the ed25519 key the connection is addressed by -- the same key whether it reached the node directly or through a tunnel. Generic rather than per-endpoint because what the names mean belongs to the storage server, not here. Report an established connection alongside it. The far end keys a subscription on the connection, so a reconnect silently drops it, and until now nothing said a connection had come back -- only that one had failed, via a listener that never fired.
Whether a subscription is worth making is a property of the routing mode, which Core has no way to see: it holds a Network and the router type lives in that Network's config.
Whatever the far end holds for a connection dies with it, and the only thing that said so was the per-node, one-shot failure listener the onion router uses to retire a path. A subscription needs the general form: every connection this transport loses, reported for as long as anyone is listening.
Everything from the `public:` above it was already public: the only specifiers in between belong to the nested AccountSeedAccess, which come after and do not change the enclosing class's access.
A client that has drained a node's namespaces has an established connection and current cursors, which is the only state a subscription can safely start from: subscribe before that and the gap between the last retrieve and the subscription taking effect is lost. So the existing poll is what both chooses the node and prepares it, and the subscription starts where the drain finishes. From there the node pushes each new message and the poll ticker stops. Renewal runs every 30s, far inside the server's 65 minute expiry, because renewing is not all the timer is for: a subscribed client sends nothing else, so the tick's retrieve is also the only thing that can notice the node has stopped holding our swarm. Losing the connection gives the subscription up -- the far end keys it to the connection and says nothing when it lapses -- and polling resumes, which is also what picks the next node. Subscribes with d=1 so a notification carries the message rather than just its metadata: the same bytes a retrieve would have returned, so no round trip and nothing to fetch. Nothing runs at all under onion requests, where the server would key the subscription to the last relay rather than to us.
A subscription that has stopped applying is silent. The storage server runs no swarm check when subscribing and none when a swarm moves underneath one: get_notifiers simply stops matching, so a client that has given up polling cannot tell "nothing has been sent to me" from "I am subscribed to a node that no longer holds my messages". So ask it something, every 30s, purely for the error. The cheapest question that still produces a 421 is a retrieve of a namespace that needs no signature -- the server decides wrong-swarm from the pubkey on the first two lines of the handler, before the auth check -- and of one nothing is ever stored in, so there is no cursor either: 97 bytes out, 57 back, against 3.3kB for the full poll this replaces. The request carries no swarm_pubkey, which is what stops the network layer helpfully retrying the 421 on a different member and reporting success. Renewal is a separate 15 minute timer now that it no longer has to carry the probe: the server's expiry is 65 minutes, and it only applies to a connection that has stayed up that long, since losing the connection loses the subscription outright. The probe is temporary. The storage server is gaining a notification that says outright when a subscription has stopped applying and carries the replacement swarm with it; this has to outlive the last node without it.
…lback _drop_subscription is reachable from inside the tickers it destroys -- _subscription_probe calls it directly -- and Loop::call_every hands out a shared_ptr whose deleter is a call_get of the delete, which runs inline once we are already on the loop. Dropping the last reference there would free the std::function being executed and return into it. Stopping the event is safe from within it; freeing the object is not, so hand it to Loop::reset_soon, which exists for this.
Session Router never reported its paths at all -- get_active_paths() was a stub returning nothing -- so the mode that is the default showed the user nothing about where their traffic went. Filling that in meant not reusing the type the routers keep their own paths in. A hop was a service_node, which a Session Router relay is not: it has no ports, no storage server version and no swarm, so reporting one meant inventing five fields, including a swarm id of 0 that means something else. And a path's destination sat beside the hop list rather than at the end of it, as two strings in one variant's metadata and absent from the other's -- from which the destination's country, the thing being asked for, could not be looked up at all. So the user-visible shape is now its own: hops of an identity and an address, in order, and nothing that only means something inside a router. What the last hop is depends on the route and the comment says so, rather than the type promising a destination that is sometimes a guess. The question changes with it. Enumerating paths suited the onion router's pools and nothing else: Session Router holds a session to every swarm member we have spoken to, the file server and every group's swarm, so listing them buries the one route anybody wants in dozens nobody can act on. Asking about a destination is answerable by all three routers -- direct returns the node itself, one hop -- and lets each resolve internally what it used to hand over and ask the caller to filter. The C wrapper for the old call is deleted rather than followed across: it exists for Session versions that predate the Client API and no client built on Client uses it.
get_path_to picked _paths[standard].front(), which is not the path a request goes down: selection skips struck paths, skips any path containing the destination, and then orders by how busy each is. The answer looked plausible and was usually wrong -- and the destination, which I had ignored as irrelevant to an onion path, is exactly what the conflict check turns on. Split the destination and category out of _find_valid_path's Request so it can be asked without one, and ask it.
Session Router is onion routing too -- it is the more capable of the two, and the mode push notifications exist for -- so "an onion path" as shorthand for an onion-request path is not loose, it is wrong. The push hook's comment read as though onion routing could not receive pushes, when it is specifically onion_requests that cannot.
Network re-aimed a request by itself: a 421 picked a different swarm member, an unreachable node walked to the next one, and either way the caller's callback fired for a node it was never told about. So Core recorded retrieve cursors against the node it asked rather than the one that answered, and would subscribe to a node that had just said it does not hold our account. Nothing below Core can fix that, because nothing below Core knows the substitution matters. That division made sense when session-ios was the main consumer and the logic had to live under the C API to be shared at all. With Client there is a better place for it. So Network stops deciding. Both retries are gone, along with Request::retry_421_count, Request::failed_nodes and the redirect_retry_count option that bounded one of them; a 421 and an unreachable node are now reported to the caller, distinguished by status code, and what to do about either is the caller's. Two things had to change to make that possible: Network now reports the collapsed batch status rather than the raw transport one. A batch whose subrequests all failed identically arrives as a transport-level 200, so a caller could not previously tell a misdirected poll from any other failure -- which is precisely the distinction it now has to make. And Network still adopts the swarm a 421 carries, because that is its own cache and the answer is authoritative; it simply does not act on it. Whoever retries then resolves against corrected membership instead of the stale set that misdirected them, which is the swarm correction that has never happened until now. Core gains _swarm_request to make those decisions in one place, and it rebuilds the request body per attempt: a retrieve carries the chosen node's cursor, and the old path re-sent one node's cursor to another.
_poll resolved the swarm and picked a member itself, then handed that member to _send_poll and to everything downstream -- including the hash cursors and the subscription -- regardless of which member the network layer had actually reached. It now goes through _swarm_request, which reports that. Building the batch moves into _build_poll_body so the helper can rebuild it per attempt. That matters here more than anywhere else: the batch carries one cursor per namespace, and those cursors belong to a particular member, so a re-aimed poll built from the old member's body would ask the new one to resume from a position it never issued. A continuation round pins the member it is continuing against, for the same reason, and falls back to choosing normally if that member has become unusable.
The PFS retrieve, delete, store and config push each resolved a swarm, picked its first member and sent, and each would now simply fail on a 421 that Network no longer recovers from. They go through _swarm_request instead, which re-aims for them. None of them needs to know which member answered -- only the poll records anything per-node -- but they all need the retrying, and having one implementation of it is the point. The subscribe is deliberately left sending directly: it is aimed at the member whose namespaces were just drained, and re-aiming it elsewhere would subscribe to a node other than the one Core is tracking. A failure there drops the subscription and returns to polling, which re-picks anyway. Configs needed naming as a friend: friendship does not reach a component through detail::CoreComponent.
test_swarm_retry drove Network's retry against a scripted router. That retry is Core's now, so the file tests Core instead -- through the poll, which is a real caller rather than a harness, so what is asserted is what a caller actually gets. MockNetwork grows two things to make that possible: a multi-member swarm, since it only ever returned one node, and an optional auto_reply so a test can script answers per member instead of firing every stored callback by hand. Writing them found a real defect. A member answering 421 was not recorded as spent, so re-resolving could pick the very same member and be rejected again, three times over, before the redirect limit stopped it. Network's version excluded the failed node explicitly and mine had dropped that: relying on the corrected swarm to no longer contain it only works when the rejection carried one, which an older storage server does not. The list now holds both kinds of spent member and is named for that rather than for one of them. The teardown case moves to its own file, since it is about Network rather than about swarms. It reached the loop thread via the retry; with that gone it uses get_swarm, which answers from the loop for the same reason.
Draining runs before the subscription exists, so a message stored between the last retrieve's snapshot and the subscription taking effect falls between the two: too late to be returned, too early to be pushed. Nothing else covers it -- the renewal sends no retrieve, and the probe asks about a namespace that is empty by design -- so it would sit unseen until the next reconnect drained again. Aimed at the member just subscribed with, rather than choosing afresh.
The probe and the renewal are the only traffic a subscribed client makes, so whether they are still happening is the first thing worth checking when pushes stop -- and the probe said nothing at all unless it failed. The renewal was already visible through _send_subscribe.
Reported from the CLI: a reliable SIGSEGV at exit, five in ten runs, in _drop_subscription called from the connection-lost hook. Core had no destructor, so members went in reverse declaration order -- and every ticker is declared after `_network` while `_loop` is declared first. So at teardown the tickers were released, then ~Network failed the requests its transport was holding, which fired our connection-lost hook, which marshalled onto the loop that was still alive, and stopped Tickers that had already been freed. Not a narrow race: a fixed ordering, which is why it reproduced. So detach the hooks and stop the timers before anything goes. ~Network already does the same for its own router and transport, and says why. Doing it in a destructor rather than by moving the member declarations around leaves the requirement written down instead of resting on where a field happens to sit. Unverified against the crash itself: reproducing it needs a live subscription, and Session Router cannot build paths in the environment I have -- no RC found for the pivot, zero path-builds, so nothing polls and nothing subscribes.
Every storage server endpoint answers in JSON except `monitor`, which is handled outside the RPC dispatch and replies with bt. So each subscribe, and each renewal after it, logged a parse warning while trying to read a clock offset and fork versions that a bt reply does not carry anyway. Recognised and skipped rather than parsed and complained about: a subscription renews on a timer, so this was a warning every fifteen minutes for the life of the process.
The push-notification work landed while responses were still handled wherever they arrived, and left three things behind that the threading contract does not allow. Every network callback it added used `_loop.call`, so the job was the loop's and survived until `~Loop` -- reaching into components that had already been destroyed. They go on the queue like everything else, which cancels them instead. The server push was handled on the network's thread outright, with a comment arguing that this kept it serialised with poll responses, which were delivered there too. That is no longer true: poll responses are marshalled now, so a push was the only thing left merging configs and writing the database off-loop, and the comment justified the race it was written to avoid. The body has to be copied to defer it, which is what the comment was trading against and is worth paying. The tests needed the same correction rather than an exemption. A mock response fired from the test's own thread is not what production does, and `JobQueue::call` runs inline inside the loop and defers outside it, so a swarm walk driven from off-loop advanced one member per answer and then left the rest queued past the end of the test. Both entry points -- `TestHelper::poll` and the callback stored for each captured request -- now deliver where a real one does.
A config push is the only swarm request that can be large, and it shared the reserved stream with the polls, the stores, and the notifications the server sends back to us -- all of which are small, and all of which waited behind it, since ordering is per-stream. Nothing sets this yet: `swarm_request` still stamps every swarm request `standard_small`, so this changes no behaviour until a caller opts in. Both of the mode-specific categories are now labelled as such. `file` means something only under onion requests, where a file reaches the file server through a storage node and so shares that node's connection; Session Router reaches the file server directly, so there is nothing to separate it from. `config` is the other way around: it needs a real QUIC connection per storage node to have a second stream to open, which is Session Router's, and under onion requests it behaves as `standard_small`.
The push debounce was armed by releasing a Batch, which means a caller had to announce that a run of changes was over. `configs.batch()` appears at exactly one call site in the tree -- the poll handler, where it was added for its own purpose of coalescing several namespaces' merges -- and no local caller ever took one. So the only thing that armed the timer was a poll going out, and a change made locally was dumped and pushed whenever one next happened to. Under onion requests that is every few seconds, which is why nothing noticed; every restart test in the suite drives its change in through merge(), which flushes on its own path. Under a live subscription polling stops altogether, and a local change then reached neither disk nor the swarm for the life of the process. Handing out the reference is the only thing this layer sees, so that is where the settle is scheduled from. The config cannot report it instead: `dirty()` marks the config and bumps the seqno *before* the assignment that follows it, so a handler hung there would fire before the change existed, and dumping from one would persist a bumped seqno without the change and clear the flag that says it still needs writing. Scheduling for the next turn of the loop sidesteps that: by then the job holding the reference has returned and the config is whole. Batch keeps doing what it is for -- a caller who knows more is coming -- and the debounce keeps doing what it is for, which is coalescing changes with no such boundary to see. ~Batch no longer throws out of a destructor while _flush writes to the database, which universal settling makes reachable.
`on_loop()` is true throughout construction because the constructing thread is the only one that can reach a component -- but the loop thread is already running by then, so a job posted during construction runs against it concurrently. Scheduling a settle from the config accessors did exactly that: `initialise_new_account` reaches `user_profile()`, which queued a flush, which serialised the config on the loop while the constructing thread was still writing to it. It showed as an intermittent SIGSEGV -- roughly one full-suite run in three, landing in whichever test happened to be constructing a Core, with the loop thread inside `ConfigMessage::serialize` and the constructing thread inside `Globals::init`. Nothing is owed by skipping it: construction flushes what it changes itself. The destructor now writes what has not settled yet, through the queue rather than around it, which both runs whatever is still pending and puts the dump on the thread permitted to do it.
Stopping the queue is what guarantees nothing of ours runs while the components are being destroyed, and none of what they hold is thread-safe. Doing it from inside a job on that queue is what `process_job_queue` is written for: it re-checks the running flag between jobs, so nothing else in the batch runs afterwards. `stop()` also clears the queue and deletes every armed `call_later`, so no timer is left to fire either. The Network is torn down before this rather than after. Failing the requests it still holds runs their completions, which marshal onto our queue -- onto a stopped one that throws, if the order is the other way round. Queued here they are cancelled by the stop a moment later, which is what should happen to them.
`Client::_set_nickname` wrote the row and committed, and only then synced it into Contacts -- where `set_nickname` throws above MAX_NAME_LENGTH. So an over-long nickname left a committed row the config could never carry, and because a sync rebuilds the whole entry from that row, every later change to that contact went with it: a block, an approval, a priority, a delete-before instruction, none of them reaching the config again until somebody happened to set a shorter nickname. It is checked before the write now, and reported rather than corrected. `_async` turns the throw into the error the caller's handler is given, which is what an application wants to put in front of whoever typed it; silently keeping the first hundred bytes would change what they wrote. `validate_contact_name` and `fixup_contact_name` are free functions needing no account or config, so an application can check a name as it is typed rather than finding out when it tries to store one. `fixup_` is what the setters for a *received* name already did by hand -- a peer's profile name has to be stored whatever they set it to -- and now has one home for whatever else storing a name comes to require. The doc comments on set_name and set_nickname described the opposite of what they do, which is how the difference between them went unnoticed.
A swarm member is chosen by shuffling and then ranking on strike count, which only learns that a node is unreachable by failing against it first. Under Session Router that is a wasted round trip on a predictable population: a storage server older than 2.11.1 is paired with an oxend that predates the relay requirement, so it very probably has no relay running and cannot be reached that way at all. The version therefore joins the ordering ahead of nothing and behind strikes, giving three groups: preferred version, then the rest, then the struck-out. Strikes stay the outer split deliberately -- one of those is a node that has actually failed us, where the version is only a prediction. Ordering rather than filtering, and only the retained set is touched, so the existing rule about adopting struck nodes to make up the numbers is untouched and a swarm with no preferred members hands back exactly what it did before. That matters because the relay is not yet enforced: a node can be new enough and still not answer. `SnodePool` takes a version rather than a reason, so what the version means belongs to the caller -- there have been other occasions for preferring a storage server fix, and this is the knob for them. Only Session Router sets it: onion requests reach a node through relays that do not care what it runs, and `direct` talks to it straight, so in neither case does the version predict anything and preferring on it would narrow the swarm for nothing.
…s one
Groundwork only: nothing uses these yet and no existing signature
changes. `failable_function` is untouched and still what everything
takes.
`Expected<T, E = Error>` is a deliberate *subset* of std::expected, and
deliberately no more permissive than one, so that raising the standard
replaces it with an alias and touches no call site. Being stricter is
free; being looser is a trap that springs years later on whoever does the
switch, which is why the ref-qualified overloads of `operator*` and
`error()` match exactly rather than approximately: handing back a `T&`
where std::expected hands back a `T&&` is enough to make code compile now
and fail then.
Rather than a conformance test listing whichever uses someone thought of,
the header simply *is* the alias when the standard library has the real
thing. So building at -DCMAKE_CXX_STANDARD=23 compiles every use in the
project against std::expected, and there is nothing to keep in step by
hand. libsession already builds clean that way with the whole suite
passing, so the CI job added here is holding a line that is met today
rather than aspiring to one.
Error carries a code and a message: the code is what a caller acts on,
the message what it falls back to when it does not recognise the code,
and what a log gets either way. An open set of codes rather than an
enum, so that adding one is not a breaking change for anyone switching
on them.
It is `final` because a subclass handed to `Expected` would be sliced
silently, and it has no `operator bool` because that would make it
convertible to bool -- and `Expected<bool>{some_error}` would then
quietly store a successful `true`, which six of this codebase's handlers
would have been exposed to. Whether something succeeded is Expected's
question, not Error's.
The code is a string_view over static storage, so a std::string is
refused. The constraint on that deleted constructor is load-bearing: a
plain `Error(std::string, std::string) = delete` also rejects string
literals, since `const char[N]` reaches std::string and std::string_view
at equal rank and the call is ambiguous before deletion is considered.
`set(CMAKE_CXX_STANDARD 20)` creates a *normal* variable, which shadows the cache entry of the same name for the rest of the directory scope. So `-DCMAKE_CXX_STANDARD=23` was accepted without complaint, written to the cache, and then read straight past: a build configured that way compiled every one of its 366 translation units at `-std=c++20`, and the only way to notice was to go looking in compile_commands.json. Setting the cache entry instead is what leaves the knob connected, and it has to be the cache rather than a `NOT DEFINED` guard around the plain `set`, because `external/date` puts 20's competition there: it sets the same variable to 17, so on the *second* configure of a build directory the guard would find it already defined, decline to set 20, and hand the whole tree 17. Our own code survives that only because session-sqlite exports `cxx_std_20` as a PUBLIC feature requirement, which happens to raise everything that links it back to 20. Neither `set` uses FORCE, so claiming the entry first is what decides it, and an explicit `-D` beats both by being in the cache before either runs.
`std::is_enum_v` is also true of an unscoped enum, which converts to its underlying type on its own. Such a type reaching the enum branch gets parsed as an enum when every other branch would have taken it as the integer it implicitly is, so which behaviour a field gets depends on whether whoever declared it wrote `enum class`. `std::is_scoped_enum` says the narrower thing but arrived in C++23, so it is aliased where it exists and spelled out where it does not.
`failable_function<void(A...)>` put an `optional<string> error` in front of whatever the call produced, which made the failure and the value two independent things a handler had to reconcile. Nothing stopped a caller reading the value on a failure, or a callee filling in both, and the type said nothing about which combinations were meant to occur. `result_function<T>` carries an `Expected<T>` instead: either the value or an `Error` saying why there isn't one, never both and never neither. The value keeps its own type rather than being flattened alongside an error, so a handler that ignores the failure does not compile. Every declaration and call site moves over at once -- there is no period with both spellings -- and `failable_function` goes away. The errors themselves are still mostly `internal.exception` carrying an exception's `what()`, which is what the old string argument already held. What changes is that there is now somewhere for a code to go: `err::` names the two the attachment paths distinguish, and the rest can be filled in as callers turn out to need to tell failures apart.
Under C++23 `session::Expected` resolves to `std::expected` rather than to the stand-in, so these jobs compile every use in the tree against the real thing. That is what holds the stand-in to being a strict subset: a use that has drifted outside it fails here rather than waiting for whoever eventually raises the standard. Both libstdc++ and libc++, because the two disagree about plenty and a subset that only holds against one of them is not a subset.
`std::free` comes from <cstdlib>, which nothing here included. libstdc++ happens to drag it in behind one of the others, so this builds; libc++ does not, and the file does not compile against it at any standard.
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.
(Builds on #152 -- only the last few commits are the intended part of this PR)
Replace
failable_functionwith anExpected-carrying result handlerEvery asynchronous method in the public API used to hand its handler an error alongside whatever it produced:
The failure and the value were two independent things the handler had to reconcile. Nothing stopped a caller reading the value on a failure, or a callee filling in both, and the type said nothing about which combinations were meant to occur.
They now carry one argument that is either/or:
Never both, never neither, and the value keeps its own type instead of being flattened next to an error — so a handler that ignores the failure does not compile.
This is a breaking API change. All 122 declarations and call sites move over in one commit; there is no period with both spellings, and
failable_functionis gone.Expectedis deliberately a subset ofstd::expectedExpected<T, E = Error>is a stand-in that is no more permissive thanstd::expected, so that raising the standard replaces it with an alias and touches no call site. Being stricter is free; being looser is a trap that springs years later on whoever does the switch. Hence the ref-qualified overloads ofoperator*anderror()match exactly rather than approximately — handing back aT&wherestd::expectedhands back aT&&is enough to make code compile now and fail then.Rather than a conformance test listing whichever uses someone thought of, the header simply is the alias when the standard library has the real thing. Building at
-DCMAKE_CXX_STANDARD=23therefore compiles every use in the project againststd::expected, and there is nothing to keep in step by hand.ErrorCarries a code and a message: the code is what a caller acts on, the message what it falls back to when it does not recognise the code, and what a log gets either way. An open set of codes rather than an enum, so adding one is not a breaking change for anyone switching on them.
The code is a
string_viewover static storage, so astd::stringis refused. All error codes should be string_view constants somewhere, so that deliberately keeping them as statically allocated string_view helps ensure we don't use some sort of more difficult error code generation code.Errorisfinalbecause a subclass handed toExpectedwould be sliced silently.What this does not do yet
Errors are still mostly
internal.exceptioncarrying an exception'swhat()— which is exactly what the old string argument held, so no information is lost or gained. What changes is that there is now somewhere for a code to go:err::names the two the attachment paths distinguish, and the rest can be filled in as callers turn out to need to tell failures apart.Separately,
failable_function<void(int64_t message_id)>named its parameter andresult_function<int64_t>does not. 73 sites lost that; moving those names into doc comments is a follow-up, not part of this.Along the way
Let the command line choose the C++ standard—set(CMAKE_CXX_STANDARD 20)created a normal variable shadowing the cache, so-DCMAKE_CXX_STANDARD=23was accepted, written to the cache, and then read straight past; a build configured that way compiled all 366 TUs at-std=c++20. It has to be a cache entry rather than aNOT DEFINEDguard, becauseexternal/datesets the same variable to 17 — under a guard, the second configure of a build directory would find it defined, decline to set 20, and hand the tree 17. (Our code survives that today only because session-sqlite exportscxx_std_20as a PUBLIC feature requirement.)Parse only scoped enums as enums—std::is_enum_vis also true of an unscoped enum, which converts to its underlying type on its own, so which parse a field got depended on whether whoever declared it wroteenum class.Include what mnemonics uses—std::freeneeds<cstdlib>; libstdc++ drags it in behind another header and libc++ does not.This also adds two CI jobs that exercise the build in C++23 mode (so that we properly test that our custom Expected implementation falls back properly with real std::expected).