Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graceful stop in repeat mode - part 2 #833

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion doc/unison-manual.tex
Original file line number Diff line number Diff line change
Expand Up @@ -1995,7 +1995,22 @@
monitoring), interrupting with ``Ctrl-C'' or with signal \verb|SIGINT| or
\verb|SIGTERM| works the same way as described above and will additionally stop
the continuous process. To stop only the continuous process and let the last
synchronization complete normally, send signal \verb|SIGUSR2| instead.
synchronization complete normally, press ``Ctrl-D'' or send signal
\verb|SIGUSR2| instead.
Closing the input, receiving an EOF, or receiving a \verb|^D| (0x04) from the
input terminal or a redirected standard input all have the same effect. EOF is
not interpreted as a stop request when standard input is redirected from a
regular file. If the standard input is not open or is not open for reading
already at Unison startup then it is ignored and only signals can be used to
send the stop request.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have some idea that the regular file will be written, and that unison will be doing rewind and seeing the content again.

And, if there is an empty/32byte rule, it should be in the manual. Perhaps the manual should say that it must be a 0-byte file and stopping must write a single 0x4, if that's how it is, or maybe 0x4\n is ok too. The specification of what to do should be narrow. We also need to avoid UTF-8/encoding issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to have some idea that the regular file will be written, and that unison will be doing rewind and seeing the content again.

Need to improve the language. That's actually not what will happen; there will be no rewind, only appended content is read. (In fact, I thought about rewinding but it seems too much...)

And, if there is an empty/32byte rule, it should be in the manual. Perhaps the manual should say that it must be a 0-byte file and stopping must write a single 0x4, if that's how it is, or maybe 0x4\n is ok too. The specification of what to do should be narrow. We also need to avoid UTF-8/encoding issues.

Can add the 0-byte rule. Currently (can be changed, of course), only 0x4 is checked but any other input before and after that byte is ignored. So, 0x4\n will work but so will \n0x4. Encodings will cause any issues here because it's just the raw byte 0x4 that is checked.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So read(2) will return EOF, and then when a byte is written a further read(2) call with return that byte? Is that guaranteed?


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to say really clearly that if stdin is /dev/null, then the stdin checking process is not used, as a guarantee.

\begin{quote}
{\em Tips:} For continuous synchronization the input should not be redirected
from any source providing a lot of input (such as an existing regular file
larger than a few tens of bytes or a device or a pipe that produces large
quantities of data). In such cases, Unison would keep reading all the input
looking for the stop condition.
\end{quote}
\end{textui}

\SUBSECTION{Exit Code}{exit}
Expand Down
18 changes: 17 additions & 1 deletion man/unison.1.in
Original file line number Diff line number Diff line change
Expand Up @@ -310,9 +310,25 @@ or
.Sy SIGTERM
works the same way as described above and will additionally stop the continuous
process. To stop only the continuous process and let the last synchronization
complete normally, send signal
complete normally, press
.Sy Ctrl-D
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this talking about sending a literal ^D 0x4, which one would do on BSD by ^V^D, or about typing the character coded as EOF to the tty so that the reader gets EOF?

Copy link
Collaborator

@gdt gdt Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that was a question, but maybe I mean "it would be nice if this were clearer, even though I realize being precise is harder to understand for many". Perhaps "Send an EOF by pressing the key configured for that, typically ^D. Or, send a literal 0x4."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try rewording it. Literal ^D and EOF, either will work

or send signal
.Sy SIGUSR2
instead.
Closing the input, receiving an EOF, or receiving a
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really do not understand "closing the input". stdin is file descriptor zero and unison is in charge of closing/not-closing that. I think you mean closing the other half of some split pty/fifo special file to cause read(2) in unison to return EOF.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right. There may have been some situation where a close on the other end would not produce EOF on the read end but I can't remember what it was. And thinking about it now, that doesn't seem realistic either. I may have been trying to make the language assume less about the reader knowing about conditions when EOF is generated. I'll see if I can rephrase it (no promises).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to speak precisely about how unison behaves based on return values from read(2) syscall on fd 0, and not try to have it make sense to people that do not understand, when that has any fuzziness.

.Sy ^D
(0x04) from the input terminal or a redirected standard input all have the same
effect. EOF is not interpreted as a stop request when standard input is
redirected from a regular file. If the standard input is not open or is not
open for reading already at Unison startup then it is ignored and only signals
can be used to send the stop request.
.Bd -ragged -offset indent
Tips: For continuous synchronization the input should not be redirected from
any source providing a lot of input (such as an existing regular file larger
than a few tens of bytes or a device or a pipe that produces large quantities
of data). In such cases, Unison would keep reading all the input looking for
the stop condition.
.Ed
Copy link
Collaborator

@gdt gdt Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but it seems that whatever is provided for stdin should produce essentially nothing until it either sends EOF or 0x4, and anything else is odd. This phrasing hints at usages with moderate amounts of data making sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is somewhat unnecessary, yes. I'll try to rephrase or get rid of it completely. I thought about warning users not to shoot themselves in the foot but the same is true for any stdin redirection, so nothing special here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, just dropping it seems fine.

.Sh ENVIRONMENT
.Bl -tag
.It Ev UNISON
Expand Down
2 changes: 1 addition & 1 deletion src/external.ml
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ let readChannelsTillEof l =
l

let runExternalProgram cmd =
if Util.osType = `Win32 && not Util.isCygwin then begin
if Lwt_unix.impl_platform = `Win32 then begin
debug (fun()-> Util.msg "Executing external program windows-style\n");
let c = openProcessIn ("\"" ^ cmd ^ "\"") in
let log = Util.trimWhitespace (readChannelTillEof c) in
Expand Down
4 changes: 4 additions & 0 deletions src/lwt/generic/lwt_unix_impl.ml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ therefore have the following limitations:
time, this could result in a dead-lock.
- [connect] is blocking
*)
let impl_platform = `Generic

let windows_hack = Sys.os_type <> "Unix"
let recent_ocaml =
Scanf.sscanf Sys.ocaml_version "%d.%d"
Expand Down Expand Up @@ -204,6 +206,8 @@ let wait_read ch =
inputs := (ch, `Wait res) :: !inputs;
res

let wait_read' = wait_read

let wait_write ch =
let res = Lwt.wait () in
outputs := (ch, `Wait res) :: !outputs;
Expand Down
7 changes: 7 additions & 0 deletions src/lwt/lwt_unix.mli
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ val read : file_descr -> bytes -> int -> int -> int Lwt.t
val write : file_descr -> bytes -> int -> int -> int Lwt.t
val write_substring : file_descr -> string -> int -> int -> int Lwt.t
val wait_read : file_descr -> unit Lwt.t
val wait_read' : Unix.file_descr -> unit Lwt.t
val wait_write : file_descr -> unit Lwt.t
val pipe_in : ?cloexec:bool -> unit -> file_descr * Unix.file_descr
val pipe_out : ?cloexec:bool -> unit -> Unix.file_descr * file_descr
Expand All @@ -55,3 +56,9 @@ type lwt_in_channel

val intern_in_channel : in_channel -> lwt_in_channel
val input_line : lwt_in_channel -> string Lwt.t

(* Not all functions are implemented on Win32. [impl_platform] indicates
which implementation has been built. This value must not be used to
detect the OS platform; it's intendend to be used for guarding code
which uses functions that are not implemented on Win32. *)
val impl_platform : [ `Generic | `Win32 ]
2 changes: 2 additions & 0 deletions src/lwt/win/lwt_unix_impl.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
conditions...
(we have the first, scan the subsequent ones)
*)
let impl_platform = `Win32

let no_overlapped_io = false
let d = ref false
Expand Down Expand Up @@ -335,6 +336,7 @@ if !d then prerr_endline "ACCEPT";
(****)

let wait_read ch = assert false
let wait_read' = wait_read

let wait_write ch = assert false

Expand Down
53 changes: 53 additions & 0 deletions src/pty.c
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,59 @@ CAMLprim value c_openpty(value unit) {

#endif

#ifndef _WIN32

#include <unistd.h> // pgrp
#include <fcntl.h>

CAMLprim value unsn_getpgrp(value unit)
{
CAMLparam0();
CAMLreturn(Val_int(getpgrp()));
}

CAMLprim value unsn_tcgetpgrp(value fd)
{
CAMLparam1(fd);
pid_t pgrp = tcgetpgrp(Int_val(fd));
if (pgrp == -1) {
caml_uerror("tcgetpgrp", Nothing);
}
CAMLreturn(Val_int(pgrp));
}

CAMLprim value unsn_fd_readable(value fd)
{
CAMLparam1(fd);
int fl = fcntl(Int_val(fd), F_GETFL);
if (fl == -1) {
caml_uerror("fcntl", Nothing);
}
CAMLreturn(Val_bool(!(fl & O_WRONLY)));
}

#else // _WIN32

CAMLprim value unsn_getpgrp(value unit)
{
CAMLparam0();
CAMLreturn(Val_int(-1));
}

CAMLprim value unsn_tcgetpgrp(value fd)
{
CAMLparam0();
CAMLreturn(Val_int(-1));
}

CAMLprim value unsn_fd_readable(value fd)
{
CAMLparam0();
CAMLreturn(Val_bool(1));
}

#endif

#ifdef _WIN32

#include <windows.h>
Expand Down
20 changes: 20 additions & 0 deletions src/strings.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2785,6 +2785,26 @@ let docs =
\032 cleanup procedures and terminates the process forcibly (similar to\n\
\032 SIGKILL). Doing so may leave the archives or replicas in an\n\
\032 inconsistent state or locked.\n\
\032 When synchronizing continuously (time interval repeat or with\n\
\032 filesystem monitoring), interrupting with \226\128\156Ctrl-C\226\128\157 or with signal\n\
\032 SIGINT or SIGTERM works the same way as described above and will\n\
\032 additionally stop the continuous process. To stop only the\n\
\032 continuous process and let the last synchronization complete\n\
\032 normally, press \226\128\156Ctrl-D\226\128\157 or send signal SIGUSR2 instead. Closing\n\
\032 the input, receiving an EOF, or receiving a ^D (0x04) from the\n\
\032 input terminal or a redirected standard input all have the same\n\
\032 effect. EOF is not interpreted as a stop request when standard\n\
\032 input is redirected from a regular file. If the standard input is\n\
\032 not open or is not open for reading already at Unison startup then\n\
\032 it is ignored and only signals can be used to send the stop\n\
\032 request.\n\
\n\
\032 Tips: For continuous synchronization the input should not be\n\
\032 redirected from any source providing a lot of input (such as an\n\
\032 existing regular file larger than a few tens of bytes or a device or\n\
\032 a pipe that produces large quantities of data). In such cases,\n\
\032 Unison would keep reading all the input looking for the stop\n\
\032 condition.\n\
\n\
Exit Code\n\
\n\
Expand Down
119 changes: 117 additions & 2 deletions src/uitext.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1248,7 +1248,106 @@ let setupSafeStop () =
if ok then stopPipe := Some (Lwt_unix.pipe_in ~cloexec:true ()))
end

(*** Requesting safe termination via stdin ***)

let nonEofStopCond s =
String.contains s '\004' (* ^D *)

(* [Unix.select] emulation in Windows does not manage to capture all
events (for fds other than sockets/files) if timeout is zero. *)
let selectTimeout = if Sys.win32 then 0.01 else 0.

let selectStdin () =
match Unix.select [Unix.stdin] [] [] selectTimeout with
| ([r1], _, _) when r1 ==(*phys*) Unix.stdin -> true
| _ -> false

let rec readStdinNoIntr b l =
try Unix.read Unix.stdin b 0 l
with Unix.Unix_error (EINTR, _, _) -> readStdinNoIntr b l

external fdIsReadable : Unix.file_descr -> bool = "unsn_fd_readable"

let stdinIsatty = Unix.isatty Unix.stdin

let stdinIsRegfile, stdinOkForStopReq =
let readingOk = function
| { Unix.LargeFile.st_kind = S_REG; st_size; _ } when st_size > 32L -> false
| _ -> begin try fdIsReadable Unix.stdin with Unix.Unix_error _ -> true end
in
let rec notEOF st =
if stdinIsatty then true else
match selectStdin () with
| true ->
let l = 32 in
let b = Bytes.create l in
begin match readStdinNoIntr b l with
| 0 -> false
| n ->
if nonEofStopCond (Bytes.sub_string b 0 n) then requestSafeStop ();
true
| exception Unix.Unix_error _ -> true
end
| false -> true
| exception Unix.Unix_error (EINTR, _, _) -> notEOF st
| exception Unix.Unix_error (EBADF, _, _) -> true
in
match Unix.LargeFile.fstat Unix.stdin with
| { st_kind = S_REG; _ } as st -> true, readingOk st
| st -> false, readingOk st && notEOF st
| exception Unix.Unix_error _ -> false, false

external getpgrp : unit -> int = "unsn_getpgrp"
external tcgetpgrp : Unix.file_descr -> int = "unsn_tcgetpgrp"

let isBackgroundProcess () =
try
stdinIsatty && (getpgrp () <> tcgetpgrp Unix.stdin)
with Unix.Unix_error ((EBADF | ENOTTY), _, _) -> false

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the /dev/null check, that if you get EOF on the very first read, then it isn't used? I didn't figure that out from the manual, and it provides a race condition where termination might be signalled before unison does the first read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct. Yes, this is the check and yes, there is a race condition. However, I decided to accept this race condition because it is extremely unlikely to occur with "normal" usage. I don't know of another way of checking specifically for /dev/null and distinguishing it from something else that just happens to already return EOF.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That may or may not be ok, but the documentation did not explain the behavior or caution, and did not hint at a race.

let ignoreSignal signa f =
let prev =
try Some (Sys.signal signa Signal_ignore)
with Invalid_argument _ | Sys_error _ -> None in
let restoreSig () =
match prev with
| None -> ()
| Some prev -> try Sys.set_signal signa prev with Sys_error _ -> ()
in
try let r = f () in restoreSig (); r
with e ->
let origbt = Printexc.get_raw_backtrace () in
restoreSig ();
Printexc.raise_with_backtrace e origbt

let isStdinStopCond n s =
(n = 0 && not stdinIsRegfile) (* Assuming terminal_io.c_vmin > 0, this is true EOF *)
|| nonEofStopCond s

let readStopFromStdin () =
let l = 32 in
let b = Bytes.create l in
let ignoreTtin f = if stdinIsatty then ignoreSignal Sys.sigttin f else f () in
try
let n = ignoreTtin (fun () -> readStdinNoIntr b l) in
isStdinStopCond n (Bytes.sub_string b 0 n)
with
| Unix.Unix_error ((EAGAIN | EWOULDBLOCK), _, _) -> false
| Unix.Unix_error (EIO, _, _) -> false
| Unix.Unix_error (EBADF, _, _) -> true

let rec checkStdinStopReq () =
if not stdinOkForStopReq || isBackgroundProcess () then () else
match selectStdin () with
| true ->
if readStopFromStdin () then requestSafeStop ()
else if not stdinIsRegfile then checkStdinStopReq ()
| false -> ()
| exception Unix.Unix_error (EINTR, _, _) -> checkStdinStopReq ()
| exception Unix.Unix_error (EBADF, _, _) -> ()

let safeStopRequested () =
if not (safeStopReqd ()) then checkStdinStopReq ();
safeStopReqd ()

(*** Sleep interruptible by a termination request ***)
Expand All @@ -1260,15 +1359,31 @@ let safeStopWait =
| None -> Lwt.wait ()
| Some (i, _) -> Lwt_unix.wait_read i
in
let winFd = ref None in
let winReadStdin () =
let (b, fd) = match !winFd with Some x -> x | None ->
let x = (Bytes.create 32, Lwt_unix.of_unix_file_descr Unix.stdin) in
winFd := Some x; x in
Lwt_unix.read fd b 0 (Bytes.length b) >>= fun n ->
if (isStdinStopCond n (Bytes.sub_string b 0 n)) then
requestSafeStop ();
Lwt.return ()
in
let readFail = function
| Unix.Unix_error (EBADF, _, _) -> Lwt.return (requestSafeStop ())
| e -> Lwt.fail e
in
let rec loop () =
let waitStdin () =
if not stdinOkForStopReq then Lwt.wait ()
else if Lwt_unix.impl_platform = `Win32 then winReadStdin ()
else Lwt_unix.wait_read' Unix.stdin
in
Lwt.catch
(fun () -> readStop) readFail >>= fun () ->
(fun () -> Lwt.choose [waitStdin (); readStop]) readFail >>= fun () ->
if not (safeStopRequested ()) then
Lwt_unix.sleep 0.15 >>= loop
(if stdinIsRegfile || isBackgroundProcess () then 1. else 0.15)
|> Lwt_unix.sleep >>= loop
else
Lwt.return ()
in
Expand Down