Repository navigation
Keep an ordinary process's exit for its owner when it missed it - #93
Conversation
A WAIT on a process its session holds no route to looks at it with a temporary WATCH. That look is refused as CONFLICT while the process's exit is on its way to its watchers (the record is final only once they all have it), or while the session's own previous look, such as the one a KILL's CONTROL just made, is still leaving. So a WAIT right after a KILL of a detached process could answer CONFLICT instead of the exit: client_host's a_process_whose_attachment_went_is_waited_for_and_held_by_nothing failed so on a loaded machine. Such a WAIT now waits for the process catalogue's next change, with which either settles, and looks again, within its timeout. a_wait_right_after_a_kill_of_a_detached_process_gets_its_exit (64 rounds of spawn detachable, detach, KILL, WAIT) failed 1 run in 10 without the change, and none in 20 with it. a_watcher_whose_queue_fills_after_the_exit_fails_alone took only output in its frame loop and panicked on the stdin's progress event, which a slow machine delivers among the frames; it lets it pass now, and waits for the child's reaping (Server::wait_reaped, tests only) rather than 500 ms.
A WAIT's temporary look can be refused as CONFLICT because this session's own look at the process is still bound: a concurrent CONTROL's, or a route that an eviction failed and has yet to detach (dispatch_outbound fails the route, then sends the Detach). Such a WAIT waited for the catalogue's next change, which a Detach doesn't make, so it slept until the process ended and then found nothing of an ordinary process: NOT_FOUND. It now waits with Manager::wait_native_look, which also wakes when the record changes and returns once nothing refuses a look: no exit in flight and no binding of this endpoint. a_wait_that_finds_its_own_look_still_bound_answers_once_it_goes fails a watcher's route, WAITs, and detaches the look 200 ms later. Without the change the WAIT answered NOT_FOUND once the child's second had passed; with it, the exit. The Exit arm's comment still said such a WAIT answers CONFLICT; it says what it does now.
An ordinary process belongs to its spawning session. yas-client WAITs for its exit when the attachment that would report it goes first: a stream reset or dropped, a DETACH, or a connection that drops and comes back just as the process exits. That WAIT answered NOT_FOUND whenever the exit came in between. With no binding of the owner's to take the exit, the record was released and nothing was kept, since only a detachable process's final is retained. The same happened in two other cases. When the owner's route failed as the exit was queued, the adapter dropped the exit because no route was there to take it. When the exit found the owner's event queue full, it was dropped, and the attachment waited for it forever. The owner now finds the exit in each case: - The adapter records the replay of every exit it is handed, whether its route is there or not. NativeEvent::Exit now carries the process handle for that. - For the owner's endpoint alone, the native side keeps the final of an ordinary process whose exit that endpoint missed: it had no binding when the exit was queued, or its exit event was dropped before dispatch. WriterGuard now tells its action whether its event was dispatched. The endpoint keeps the newest exit_replays() of these finals, as many as the adapter keeps replays, until it shuts down. WATCH, and so WAIT and ATTACH, finds them after the public finals. They are stored before the release moves the catalogue, which is what a WAIT that found the exit on its way waits for. - A binding whose exit can't be queued is evicted, as a binding that falls behind is. Its attachment fails and its client WAITs, instead of waiting for an exit that won't come. New tests, each of which failed with NotFound before this change: - an_owner_whose_look_went_before_the_exit_still_waits_for_it - an_owner_whose_route_failed_as_the_exit_came_still_waits_for_it - an_owner_whose_queue_was_full_as_the_exit_came_still_finds_it an_owner_that_drops_its_stream_after_the_exit_still_waits_for_it no longer depends on whether its detach comes before or after the exit. a_watcher_whose_queue_fills_after_the_exit_fails_alone assumed that 80 lines make 80 frames, which breaks when a loaded reader takes several lines in one read. Its child now writes each line only after the test has received the last one, as a FIFO tells it, and runs without stdin, so there are no stdin events. The test checks each frame, and checks that only the watcher's attachment failed, evicted.
There was a problem hiding this comment.
Ultimator's review of 45ff11e
Keeps an ordinary process's final for its owner when the owner missed the exit: no binding, the exit dropped before dispatch, or its route gone in the adapter, which now records every exit's replay before it looks for the route. It also evicts a binding whose exit finds its queue full. The server change checks out against the code, and the new and changed yas_process tests pass on their own. One new test is racy: under load it failed 7 of 24 parallel runs of yas_process::tests::a, and 3 of 40 parallel runs of the test alone.
1 issue: 1 bug
No issue is open since the review of 29350d9.
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
The failed try_send drops the exit's envelope inside send_native, whose guard keeps the final and releases the record before try_queue_terminal evicts the binding: the test could see the process gone and the final kept with no eviction pushed yet. It waits for the eviction's notice (a permit is kept when it came first). Under load, it failed 7 of 24 parallel runs of yas_process::tests::a, and 3 of 40 alone, in review; with the wait, none of 24 and none of 40.
|
Reviewed |
ghost
left a comment
There was a problem hiding this comment.
Ultimator's review of 29350d9
The new commit waits on evictions.notified() before counting the evictions, which closes the race in the queue-full test. On the test binary built from 29350d9, the same load that failed 7 of 24 runs at 45ff11e now passes: 24 of 24 parallel yas_process::tests::a runs and 40 of 40 runs of the test alone. Nothing new.
No issues found · 1 resolved
Session · @ultimator-agent review reviews again · @ultimator-agent fix fixes the open issues
Stacked on #92. Its two commits come first here. Review the last one, "Keep an ordinary process's exit for its owner when it missed it". It goes after #92: its WAIT looks again after a CONFLICT, and this PR relies on that.
The problem
An ordinary process belongs to its spawning session. yas-client sends WAIT for its exit when the attachment that would have reported it goes first. That happens on a stream reset or drop, on a DETACH, and when a connection drops and reconnects just as the process exits. If the exit came in between, that WAIT answered NOT_FOUND, and the exit was lost:
finish_terminal→release_record: removed fromlive, with nothing kept, because only a detachable process goes tofinals.This is known follow-up 1 in #92, and
an_owner_that_drops_its_stream_after_the_exit_still_waits_for_itflaked on it. That test failed 1 of 4 yas-server lib runs on #92's branch, with NOT_FOUND.The change
dispatch_outboundrecords the replay of every exit it is handed, before it looks for the route and whether or not the route is there.NativeEvent::Exitnow carriesprocess_handlefor that.EndpointState.missedholds the finals of this endpoint's ordinary processes whose exits it missed.try_queue_terminalworks out whether the owner has a binding among those that take the exit.WriterGuard's action is now told whether its event was dispatched, so an owner exit that is dropped before dispatch also counts as missed.finish_terminalstores the final beforerelease_recordmoves the catalogue. A WAIT that found the exit in flight (CONFLICT, Answer a WAIT that looks too soon with the exit, not CONFLICT #92) waits for that change, so when it looks again it finds the final.watch_nativechecksmissedafter the publicfinals. WAIT, and an owner's ATTACH, then get the exit, exactly as for a detachable process's final.Design choices, for review
ProcessMaxima::exit_replays()(the process generations maximum, never fewer than 64), until the endpoint shuts down. This is the same bound, and the same lifetime, as the adapter's per-session exit replays. There are no timers; the alternative was a TTL like the 5-minute detached-result one.Tests
New, each failing with
NotFoundbefore the change:an_owner_whose_look_went_before_the_exit_still_waits_for_it(a)an_owner_whose_queue_was_full_as_the_exit_came_still_finds_it(b): a native endpoint whose one-event queue the output fills. It also checks the eviction.an_owner_whose_route_failed_as_the_exit_came_still_waits_for_it(c)Reworked (#92's known follow-up 2):
a_watcher_whose_queue_fills_after_the_exit_fails_aloneassumed 80 lines make 80 frames. A loaded reader breaks that by taking several lines in one read.x\nat its offset), and checks that thezread after the exit evicted only the watcher's attachment (ROUTE_EVICTED). The watcher's session goes on.an_owner_that_drops_its_stream_after_the_exit_still_waits_for_it: its comment now says why it holds whether the detach comes before or after the exit.Checks
cargo fmt --check: passes.cargo clippy -p yas-server --all-targets -D warnings: clean.a_wait_that_finds_its_own_look_still_bound_answers_once_it_goesanda_wait_right_after_a_kill_of_a_detached_process_gets_its_exit: 10/10 each.All on a machine at load 50–290.
Fork CI: see the linked indent-com draft.