Give Core's components a threading contract, and enforce it - #158
Open
jagerman wants to merge 5 commits into
Open
Give Core's components a threading contract, and enforce it#158jagerman wants to merge 5 commits into
jagerman wants to merge 5 commits into
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.
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.
Clientdispatches every one of its methods onto Core's loop. Core's own components don't, and nothing marks the difference:client.set_display_name(name, cb)is correct, andclient.core.configs.user_profile().set_name(name)compiles just as cleanly and is a data race. The comment oncore.globalsinvites the second outright ("can also be used by the application to persist settings").The database isn't the problem — session-sqlite hands each thread its own connection and says to use it that way. What's unsynchronised is everything a component holds beside its tables:
_adopt_seedrewrites astd::stringand asecure_bufferthat the loop reads while polling, and the config objects are built lazily and then mutated bymerge(). There isn't an atomic or a mutex anywhere inglobals,configsordevices.So
CoreComponentgrowson_loop(), and everything that touches component state asserts it. Where an application legitimately calls one of these from its own thread, the method now comes in the two formsClientalready uses — afailable_functionand anawait_t— and no third:call_getruns inline when it is already the loop thread, so code already there uses the blocking form and pays nothing for it.API change
Client::loopis removed. It read as the obvious way to defer work and was the wrong one — see the lifetime fixes below.ClientandCorenow havecall/call_soon/call_later/call_getinstead, and those are what everything insideClientuses.core.loop()stays public, and now documents what it costs: a job left on the loop's own queue is not discarded until~Loop, which is the last thing~Coredoes. It is the escape hatch, not the default.Two live bugs, fixed here
The assertion found both on the first run. They are in this PR because the suite doesn't pass without them.
Networkbuilds its ownquic::Loop, sosend_requestcompletions run on its thread.Core::_send_pollcalled_handle_poll_responsestraight from one — merging configs and flushing their dumps off Core's loop, on every poll.Configs::_send_pushdoes the same on every config push: clears_push_in_flight, confirms the pushed configs, and dumps them, all from the network's thread.Both are now marshalled onto Core's queue, with the body split out (
_handle_push_response) so the callback is one line.Note what does not change: the
_alivecanary on those callbacks stays, and now says why. The Network owns them, so they can outlive Core entirely, and a stopped queue cannot cancel something that was never queued — the canary is what makes reaching the queue safe in the first place, and the queue takes over from there.Lifetime
Three fixes falling out of the same reasoning:
Coregains aJobQueueof its own, declared so that it stops — cancelling outstanding component work — before the components those jobs reach are destroyed, and while_loopis still alive to process the stop.Client::_jqexisted to do exactly that and was carrying one job._asyncand nine other sites deferred onto the loop's own queue instead, which is not emptied until~Loop. Every one of those jobs holdsthisand reaches through it into Core's members, so they were being drained throughout the destruction of every one of them.call_getstays on the loop: its caller is blocked inside it and cannot have gone away._poll_tickerwas declared with the rest of the polling machinery, so it was destroyed after the components a firing poll reaches. It is declared last now, and before_jq, so that a poll cannot try to queue its response onto a queue that has already stopped.Tests
Migrated to run on the loop, the way an application would —
TestHelper::on_loopwraps a block, andin_configscovers the tests that reach pastClientto check what was written underneath. Both hand back values: a reference or astring_viewinto a config is dangling the moment the excursion ends, which several of these were relying on.Three tests were already doing this hop by hand with
loop().call_get, which is where the idea came from.392 test cases pass.