| Message ID | 20220127215553.1386024-1-robin.jarry@6wind.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] receive-pack: check if client is alive before completing the push | expand |
Robin Jarry <robin.jarry@6wind.com> writes: > Abort the push operation (i.e. do not migrate the objects from temporary > to permanent storage) if the client has disconnected while the > pre-receive hook was running. > > This reduces the risk of inconsistencies on network errors or if the > user hits ctrl-c while the pre-receive hook is running. > > Send a keepalive packet (empty) on sideband 2 (the one to report > progress). If the client has exited, receive-pack will be killed via > SIGPIPE and the push will be aborted. This only works when sideband* > capabilities are advertised by the client. > > Signed-off-by: Robin Jarry <robin.jarry@6wind.com> > --- > v2 -> v3: > I had missed Documentation/technical/pack-protocol.txt. Using > sideband 2 to send the keepalive packet works. Yes, as long as sideband capability is supported (which is true everywhere these days), this would be good. Simple and sensible. Thanks. > builtin/receive-pack.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 9f4a0b816cf9..8b0d56897c9f 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands, > return; > } > > + /* > + * Send a keepalive packet on sideband 2 (progress info) to ensure that > + * the client has not disconnected while pre-receive was running. > + */ > + if (use_sideband) { > + static const char buf[] = "0005\2"; > + write_or_die(1, buf, sizeof(buf) - 1); > + } > + > /* > * Now we'll start writing out refs, which means the objects need > * to be in their final positions so that other processes can see them.
Junio C Hamano, Jan 28, 2022 at 02:19: > Yes, as long as sideband capability is supported (which is true > everywhere these days), this would be good. > > Simple and sensible. > > Thanks. Thank you for your patience :)
Robin Jarry <robin.jarry@6wind.com> writes: > Abort the push operation (i.e. do not migrate the objects from temporary > to permanent storage) if the client has disconnected while the > pre-receive hook was running. > > This reduces the risk of inconsistencies on network errors or if the > user hits ctrl-c while the pre-receive hook is running. > > Send a keepalive packet (empty) on sideband 2 (the one to report > progress). If the client has exited, receive-pack will be killed via > SIGPIPE and the push will be aborted. This only works when sideband* > capabilities are advertised by the client. If they have already exited but the fact hasn't reached us over the network, the write() will succeed to deposit the packet in the send buffer. So I am not sure how much this would actually help, but it should be safe to send an unsolicited keepalive as long as the other side is expecting to hear from us. When either report_status or report_status_v2 capabilities is in effect, we will make a report() or report_v2() call later, so we should be safe. > Signed-off-by: Robin Jarry <robin.jarry@6wind.com> > --- > v2 -> v3: > I had missed Documentation/technical/pack-protocol.txt. Using > sideband 2 to send the keepalive packet works. > > builtin/receive-pack.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 9f4a0b816cf9..8b0d56897c9f 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands, > return; > } > > + /* > + * Send a keepalive packet on sideband 2 (progress info) to ensure that > + * the client has not disconnected while pre-receive was running. > + */ I suspect that any keepalive, unless it expects an active "yes, I am still alive" response from the other side, is too weak to "ensure". I guess "to notice a client that has disconnected (e.g. killed with ^C)" is more appropriate. > + if (use_sideband) { > + static const char buf[] = "0005\2"; > + write_or_die(1, buf, sizeof(buf) - 1); > + } Observing how execute_commands() and helper functions report an error to the callers higher in the call chain, and ask them to abort the remainder of the operation, I am not sure if write_or_die() is appropriate. Side note: inside copy_to_sideband(), which runs in async, it is a different matter (i.e. the main process on our side is not what gets killed by that _or_die() part of the call), but this one kills the main process. The convention around this code path seems to be to fill explanation of error in cmd->error_string and return to the caller. In this case, the error_strings may not reach the pusher via report() or report_v2() as they may have disconnected, but calling the report() functions is not the only thing the caller will want to do after calling us, so giving it a chance to clean up may be a better design, e.g. if (write_in_full(...) < 0) { for (cmd = commands; cmd; cmd = cmd->next) cmd->error_string = "pusher went away"; return; } Yes, the current code will not actually use the error string in any useful way in this particular case, since report() or report_v2() will have nobody listening to them. But being consistent will help maintaining the caller, as it can later be extended to use it locally (e.g. log the request and its outcome, check which cmd has succeeded and failed using the NULL-ness of cmd->error_string, etc.)
Junio C Hamano, Jan 28, 2022 at 18:52: > If they have already exited but the fact hasn't reached us over the > network, the write() will succeed to deposit the packet in the send > buffer. So I am not sure how much this would actually help, but it > should be safe to send an unsolicited keepalive as long as the other > side is expecting to hear from us. When either report_status or > report_status_v2 capabilities is in effect, we will make a report() > or report_v2() call later, so we should be safe. This is not perfect but I think this is the best we can do without adding a new capability so that the client sends a reply to the keepalive packet. > I suspect that any keepalive, unless it expects an active "yes, I am > still alive" response from the other side, is too weak to "ensure". > > I guess "to notice a client that has disconnected (e.g. killed with > ^C)" is more appropriate. OK, I will change that. > > + if (use_sideband) { > > + static const char buf[] = "0005\2"; > > + write_or_die(1, buf, sizeof(buf) - 1); > > + } > > Observing how execute_commands() and helper functions report an > error to the callers higher in the call chain, and ask them to abort > the remainder of the operation, I am not sure if write_or_die() is > appropriate. > > Side note: inside copy_to_sideband(), which runs in async, it is > a different matter (i.e. the main process on our side is not > what gets killed by that _or_die() part of the call), but this > one kills the main process. > > The convention around this code path seems to be to fill explanation > of error in cmd->error_string and return to the caller. In this > case, the error_strings may not reach the pusher via report() or > report_v2() as they may have disconnected, but calling the report() > functions is not the only thing the caller will want to do after > calling us, so giving it a chance to clean up may be a better > design, e.g. > > if (write_in_full(...) < 0) { > for (cmd = commands; cmd; cmd = cmd->next) > cmd->error_string = "pusher went away"; > return; > } > > Yes, the current code will not actually use the error string in any > useful way in this particular case, since report() or report_v2() > will have nobody listening to them. But being consistent will help > maintaining the caller, as it can later be extended to use it > locally (e.g. log the request and its outcome, check which cmd has > succeeded and failed using the NULL-ness of cmd->error_string, etc.) The main receive-pack process will be killed by SIGPIPE anyway but I can fill the error_string fields and return for code consistency. I'll send a v4, thanks for the review.
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 9f4a0b816cf9..8b0d56897c9f 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1971,6 +1971,15 @@ static void execute_commands(struct command *commands, return; } + /* + * Send a keepalive packet on sideband 2 (progress info) to ensure that + * the client has not disconnected while pre-receive was running. + */ + if (use_sideband) { + static const char buf[] = "0005\2"; + write_or_die(1, buf, sizeof(buf) - 1); + } + /* * Now we'll start writing out refs, which means the objects need * to be in their final positions so that other processes can see them.
Abort the push operation (i.e. do not migrate the objects from temporary to permanent storage) if the client has disconnected while the pre-receive hook was running. This reduces the risk of inconsistencies on network errors or if the user hits ctrl-c while the pre-receive hook is running. Send a keepalive packet (empty) on sideband 2 (the one to report progress). If the client has exited, receive-pack will be killed via SIGPIPE and the push will be aborted. This only works when sideband* capabilities are advertised by the client. Signed-off-by: Robin Jarry <robin.jarry@6wind.com> --- v2 -> v3: I had missed Documentation/technical/pack-protocol.txt. Using sideband 2 to send the keepalive packet works. builtin/receive-pack.c | 9 +++++++++ 1 file changed, 9 insertions(+)