From 425435ad7dda553aafb3ceec87755267033a26ef Mon Sep 17 00:00:00 2001 From: Andy C Date: Fri, 8 Mar 2024 16:08:36 -0500 Subject: [PATCH] [core] Print setpgid() files to the tracer, no stderr Job control with pipelines is a inherently racy. Zulip thread: https://oilshell.zulipchat.com/#narrow/stream/307442-nix/topic/starship.20bug.20repro This is part of issue #1822. --- core/dev.py | 6 +++--- core/executor.py | 16 ++++++++-------- core/process.py | 19 +++++++++++-------- core/util.py | 5 +++++ mycpp/mylib.py | 15 +++++++++++++++ 5 files changed, 42 insertions(+), 19 deletions(-) diff --git a/core/dev.py b/core/dev.py index ac7328a2f7..9f196d99f7 100644 --- a/core/dev.py +++ b/core/dev.py @@ -271,7 +271,7 @@ class Tracer(object): Other hooks: - Command completion starts other processes - - Oil command constructs: BareDecl, VarDecl, Mutation, Expr + - YSH command constructs: BareDecl, VarDecl, Mutation, Expr """ def __init__( @@ -490,9 +490,9 @@ def PopMessage(self, label, arg): buf.write('\n') self.f.write(buf.getvalue()) - def PrintMessage(self, message): + def OtherMessage(self, message): # type: (str) -> None - """Used when receiving signals.""" + """Can be used when receiving signals.""" buf = self._RichTraceBegin('!') if not buf: return diff --git a/core/executor.py b/core/executor.py index e81a7f77db..e9904f5821 100644 --- a/core/executor.py +++ b/core/executor.py @@ -337,10 +337,10 @@ def RunSimpleCommand(self, cmd_val, cmd_st, run_flags): # If job control is enabled, this should be true assert pgid != process.INVALID_PGID - change = process.SetPgid(pgid) + change = process.SetPgid(pgid, self.tracer) self.fg_pipeline = None # clear to avoid confusion in subshells else: - change = process.SetPgid(process.OWN_LEADER) + change = process.SetPgid(process.OWN_LEADER, self.tracer) p.AddStateChange(change) status = p.RunProcess(self.waiter, trace.External(cmd_val.argv)) @@ -375,7 +375,7 @@ def RunBackgroundJob(self, node): if UP_node.tag() == command_e.Pipeline: node = cast(command.Pipeline, UP_node) pi = process.Pipeline(self.exec_opts.sigpipe_status_ok(), - self.job_control, self.job_list) + self.job_control, self.job_list, self.tracer) for child in node.children: p = self._MakeProcess(child) p.Init_ParentPipeline(pi) @@ -395,7 +395,8 @@ def RunBackgroundJob(self, node): p = self._MakeProcess(node) if self.job_control.Enabled(): - p.AddStateChange(process.SetPgid(process.OWN_LEADER)) + p.AddStateChange( + process.SetPgid(process.OWN_LEADER, self.tracer)) p.SetBackground() pid = p.StartProcess(trace.Fork) @@ -407,8 +408,7 @@ def RunPipeline(self, node, status_out): # type: (command.Pipeline, CommandStatus) -> None pi = process.Pipeline(self.exec_opts.sigpipe_status_ok(), - self.job_control, self.job_list) - #self.job_list.AddPipeline(pi) + self.job_control, self.job_list, self.tracer) # initialized with CommandStatus.CreateNull() pipe_locs = [] # type: List[loc_t] @@ -442,7 +442,7 @@ def RunSubshell(self, node): # type: (command_t) -> int p = self._MakeProcess(node) if self.job_control.Enabled(): - p.AddStateChange(process.SetPgid(process.OWN_LEADER)) + p.AddStateChange(process.SetPgid(process.OWN_LEADER, self.tracer)) return p.RunProcess(self.waiter, trace.ForkWait) @@ -609,7 +609,7 @@ def RunProcessSub(self, cs_part): p.AddStateChange(redir) if self.job_control.Enabled(): - p.AddStateChange(process.SetPgid(process.OWN_LEADER)) + p.AddStateChange(process.SetPgid(process.OWN_LEADER, self.tracer)) # Fork, letting the child inherit the pipe file descriptors. p.StartProcess(trace.ProcessSub) diff --git a/core/process.py b/core/process.py index 38348c7810..9aa3aa9b96 100644 --- a/core/process.py +++ b/core/process.py @@ -624,17 +624,18 @@ def Apply(self): class SetPgid(ChildStateChange): - def __init__(self, pgid): - # type: (int) -> None + def __init__(self, pgid, tracer): + # type: (int, dev.Tracer) -> None self.pgid = pgid + self.tracer = tracer def Apply(self): # type: () -> None try: posix.setpgid(0, self.pgid) except (IOError, OSError) as e: - print_stderr( - 'osh: child failed to set process group for PID %d to %d: %s' % + self.tracer.OtherMessage( + 'osh: child %d failed to set its process group to %d: %s' % (posix.getpid(), self.pgid, pyutil.strerror(e))) def ApplyFromParent(self, proc): @@ -642,7 +643,7 @@ def ApplyFromParent(self, proc): try: posix.setpgid(proc.pid, self.pgid) except (IOError, OSError) as e: - print_stderr( + self.tracer.OtherMessage( 'osh: parent failed to set process group for PID %d to %d: %s' % (proc.pid, self.pgid, pyutil.strerror(e))) @@ -1186,11 +1187,13 @@ class Pipeline(Job): foo | bar | read v """ - def __init__(self, sigpipe_status_ok, job_control, job_list): - # type: (bool, JobControl, JobList) -> None + def __init__(self, sigpipe_status_ok, job_control, job_list, tracer): + # type: (bool, JobControl, JobList, dev.Tracer) -> None Job.__init__(self) self.job_control = job_control self.job_list = job_list + self.tracer = tracer + self.procs = [] # type: List[Process] self.pids = [] # type: List[int] # pids in order self.pipe_status = [] # type: List[int] # status in order @@ -1282,7 +1285,7 @@ def StartPipeline(self, waiter): for i, proc in enumerate(self.procs): if self.pgid != INVALID_PGID: - proc.AddStateChange(SetPgid(self.pgid)) + proc.AddStateChange(SetPgid(self.pgid, self.tracer)) # Figure out the pid pid = proc.StartProcess(trace.PipelinePart) diff --git a/core/util.py b/core/util.py index f95cf7127b..c9b2b12f4b 100644 --- a/core/util.py +++ b/core/util.py @@ -89,6 +89,11 @@ def __init__(self): class DebugFile(_DebugFile): + """Trivial wrapper with writeln() method + + Can we turn this into a plain mylib::File? Right now that type only exists + in C++, but it should probably exist in Python too. + """ def __init__(self, f): # type: (mylib.Writer) -> None diff --git a/mycpp/mylib.py b/mycpp/mylib.py index 50276ad29d..3973d03dfb 100644 --- a/mycpp/mylib.py +++ b/mycpp/mylib.py @@ -64,6 +64,21 @@ def print_stderr(s): print(s, file=sys.stderr) +class File: + """ + TODO: This should define a read/write interface, and then LineReader() and + Writer() can possibly inherit it, with runtime assertions + + Then we allow downcasting from File -> LineReader, like we currently do in + C++ in gc_mylib.h. + + Inheritance can't express the structural Reader/Writer pattern of Go, which + would be better. I suppose we could use File* everywhere, but having + fine-grained types is nicer. And there will be very few casts. + """ + pass + + class LineReader: def readline(self):