Hi Willy,
Thanks, this settles the shape for me: a time-based grace, no state
detection, and actconn out of the picture entirely. It also removes the
question I had about a flush hook on the exit path. If the sink holds a
job, the process simply has not reached its exit condition yet and the
flush happens in the normal poll loop, so nothing special is needed
there.
Let me write out what I understood so you can correct it before I write
any code.
1. Semantics
============
Per forwarding sink, two durations, both only meaningful once stopping
is set:
grace <ms> how long the sink may keep the worker alive
empty <ms> how long it waits for a new message while the ring is
empty
and during that window the sink behaves exactly as it does outside of
soft-stop today: it dispatches, it waits on the ring, and it reconnects.
when the sink enters its grace:
deadline_grace = now + grace
deadline_empty = unset
on each wakeup during the grace:
if the ring is not empty, or something was forwarded since the
last wakeup:
deadline_empty = unset
else if deadline_empty is unset:
deadline_empty = now + empty
if deadline_empty or deadline_grace has expired:
close the forwarders, release the job
So "the ring stayed empty for <empty> ms" is the normal way out, and
<grace> is the hard cap that covers a downstream which is slow or gone.
A message arriving at any point puts us back in the first case.
I wrote <empty> as a sliding window rather than a single deadline
computed once at stopping, because a one-shot deadline would close the
sink right after a message that arrived and was drained just before it
fired, which contradicts the paragraph above. Tell me if you meant it
one-shot.
Two definitions to pin down, since both are ambiguous as soon as you
look at the code:
- with several servers on the ring, each target keeps its own read
position, so "the ring is empty" has to be read per target: every
target has forwarded everything written to the ring. A target still
holding backlog, for instance one that is reconnecting, keeps the
empty window unarmed and we run to the grace cap. I believe that is
what you meant by wanting to wait longer when there is data.
- "when the sink enters its grace" is when its forward_task gets the
stopping broadcast, not when the stopping flag is set. With a global
"grace" configured those are different instants: soft_stop() sets
stopping immediately and do_soft_stop_now() only runs after the
global delay. Keying the sink off its own armed state rather than
off the global flag keeps the two from interacting.
One consequence worth stating plainly, since I over-claimed in the
opposite direction last time: a sliding <empty> closes on the first
quiet gap longer than <empty>. A straggler whose log lands after such a
gap, a long poll on a low-traffic instance for example, is still lost no
matter how large <grace> is. That is the trade-off you described, and I
think it is the right one; I just do not want to pretend the design
guarantees more than it does.
2. Where the job is taken
=========================
Agreed on incrementing jobs, and on taking it when the sink starts
rather than when the stopping signal arrives.
I had drafted the peers variant first, taking it on the signal like the
dont_stop latch in __process_stopping_peer_sync(), before noticing that
it reintroduces the same family of race you mentioned for peers:
do_soft_stop_now() releases the listeners' jobs in protocol_stop_now()
before broadcasting signal 0, so on an otherwise idle process jobs can
reach zero and the exit condition can be met while the sink's
forward_task is still queued and has not taken its job yet. Taking it at
startup has no such window.
A sink with no grace configured takes no job at all, so its behaviour
stays exactly today's. The only visible effect of holding one from
startup is the "Jobs" counter of "show info" being one higher per
forwarding sink, since jobs is otherwise only compared in the exit
condition.
The deadlines and the periodic re-check still belong on the sink's
forward_task, which is already woken by the stopping broadcast as it is
registered on signal 0. That is also the fix for the busy-loop I
reported: the re-check has to drive forward_task->expire, which
process_sink_forward() already manages, and never appctx->t->expire.
One job per sink rather than one per forwarding target, released when
one of the two deadlines fires.
One interaction worth mentioning: hard_stop() shuts all streams down,
ours included, sets killed and re-arms itself one second later; its
second pass logs "Some tasks resisted to hard-stop", sets killed to 2,
and the poll loop exits on that. So we never hang. But in between, with
the grace still armed, the forward_task would cheerfully reconnect the
forwarders hard_stop has just killed, which is both noisy and
self-defeating. Ending the grace as soon as killed is set avoids it.
3. Reconnecting during grace
============================
Agreed that no-reconnect is wrong. There is a detail in the way though:
process_sink_forward() allows at most one connection attempt per second
per target (sft->last_conn + 1s, last_conn being set when the session is
created). With "grace 1000", a target that loses its connection during
the window gets its next attempt up to a second after the previous one,
so at the very end of the window at best, and past it, therefore never,
at worst. The guarantee you are asking for would not actually hold at
small grace values.
Skipping the tempo for the first attempt after a disconnection during
the grace looks like the smallest change, and it keeps the one-per-
second pace for repeated failures. I would do that unless you prefer
clamping the tempo instead.
4. Naming and placement
=======================
"grace" is still taken at the global level, and it means the other end
of the same operation: global "grace <time>" delays the soft-stop itself
after SIGUSR1 (global.grace_delay). A ring-level "grace" would mean
"keep going for this long once the soft-stop has started". Having both
in one configuration file seems likely to confuse.
It is your call and I do not mind either way; I just did not want us to
discover it after the fact. If you would rather avoid it, something in
the timeout family reads naturally next to the "timeout connect" and
"timeout server" a ring section already accepts.
You also mentioned the setting could live on a TCP log line. I would
start with the ring section only, give the implicit sinks created from
"log" lines the same default as everyone else, and add a per-log-line
override later if it turns out to be wanted. Those implicit sinks are
the common case, so if you would rather have the override from the
start, say so and I will do it in the same patch.
5. Defaults
===========
One thing left to decide: whether grace defaults to zero.
Zero keeps today's behaviour for everyone and makes the whole thing
opt-in. A non-zero default fixes the common case at a fairly modest
cost: streams hold jobs anyway, so the worker already lives until its
last stream finishes, which means the sink typically only adds about
<empty> ms after the last message, and the full <grace> is only reached
when the downstream is stuck or slow.
I would still start at zero so that a backport changes nothing, and
revisit the default separately once the mechanism is in. Tell me if you
would rather have it non-zero from the start.
Separately, and independent of all this: an explicit "ring" section gets
no default "timeout connect" or "timeout server" at all, both stay at
TICK_ETERNITY unless set explicitly, while sink_new_from_logger() gives
the implicit rings 1s/5s. Giving the explicit ones the same defaults
looks like a small standalone fix, useful outside soft-stop too.
6. On the point I got wrong
===========================
You are right about the TOCTOU and I withdraw the claim. actconn == 0
does not mean no more logs, since a connection sitting in the accept
queue can be accepted right after the read. As the design no longer
looks at actconn at all, the log-forward exclusion, the drain sampling
and the ring-lock ordering argument all disappear with it. Good
riddance.
7. Where that leaves the series
===============================
1. MINOR: sink: give an explicit "ring" section the default connect
and server timeouts. Independent of the rest.
2. MEDIUM: sink: the grace mechanism above. Takes a job when the sink
starts, keeps dispatching and reconnecting during the grace,
releases on whichever deadline fires first. Fixes both (a) and (b).
3. (yours) the frontend option.
That is one patch for the sink side rather than the three I proposed
last time. Splitting "flush the backlog" from "wait for the stragglers"
would only have produced an intermediate behaviour nobody wants, and
your design covers both with one mechanism anyway.
On resuming a file-backed ring in the new process: I agree it looks
possible, and I agree about the missing read position. I would rather
leave it out of this series.
So, to summarise what I need from you: confirmation that section 1 is
what you meant, and your preference on the naming. The rest I have
proposed an answer for and will follow unless you say otherwise.
Thanks,
ygkat
