From 715e11f692032bec688b242629d44672b0122af4 Mon Sep 17 00:00:00 2001 From: AnotherTest Date: Tue, 1 Sep 2020 08:42:16 +0430 Subject: [PATCH] Shell: Fix job control and backgrounding This patchset makes the shell capable of lazily resolving and executing sequences of commands, to allow for putting logical sequences in the background. In particular, it enables And/Or/Sequence nodes to be run in the background, and consequently unmarks them as `would_execute`. Doing so also fixes job control to an extent, as jobs are now capable of having 'tails', so sequences can be put in the background while preserving their following sequences. --- Shell/AST.cpp | 78 ++++++++++++-------------------------------- Shell/AST.h | 25 +++++++++++--- Shell/Forward.h | 3 +- Shell/Job.cpp | 40 +++++++++++++++++++++++ Shell/Job.h | 53 +++++++----------------------- Shell/Parser.cpp | 4 +-- Shell/Shell.cpp | 67 +++++++++++++++++++++++++++---------- Shell/Shell.h | 3 ++ Shell/Tests/if.sh | 4 +++ Shell/Tests/valid.sh | 54 +++++++++++++++--------------- 10 files changed, 180 insertions(+), 151 deletions(-) diff --git a/Shell/AST.cpp b/Shell/AST.cpp index b71b751d7d9..658494982ef 100644 --- a/Shell/AST.cpp +++ b/Shell/AST.cpp @@ -184,22 +184,9 @@ void And::dump(int level) const RefPtr And::run(RefPtr shell) { - auto left = m_left->run(shell); - ASSERT(left->is_job()); - - auto* job_value = static_cast(left.ptr()); - const auto job = job_value->job(); - if (!job) { - // Something has gone wrong, let's just pretend that the job failed. - return job_value; - } - - shell->block_on_job(job); - - if (job->exit_code() == 0) - return m_right->run(shell); - - return job_value; + auto commands = m_left->run(shell)->resolve_as_commands(shell); + commands.last().next_chain.append(NodeWithAction { *m_right, NodeWithAction::And }); + return create(move(commands)); } void And::highlight_in_editor(Line::Editor& editor, Shell& shell, HighlightMetadata metadata) @@ -354,16 +341,11 @@ RefPtr Background::run(RefPtr shell) // all but their last subnode before yielding to this, causing a command // like `foo && bar&` to effectively be `foo && (bar&)`. auto value = m_command->run(shell)->resolve_without_cast(shell); - if (value->is_job()) { - auto job = static_cast(value.ptr())->job(); - job->set_running_in_background(true); - job->set_should_announce_exit(true); - return value; - } + ASSERT(!value->is_job()); auto commands = value->resolve_as_commands(shell); - auto& last = commands.last(); - last.should_wait = false; + for (auto& command : commands) + command.should_wait = false; return create(move(commands)); } @@ -925,7 +907,7 @@ void Execute::for_each_entry(RefPtr shell, Function shell, Function last_job; + auto jobs = shell->run_commands(commands); - for (auto& job : shell->run_commands(commands)) { - shell->block_on_job(job); - last_job = move(job); - } - - callback(create(move(last_job))); - - return; + if (!jobs.is_empty()) + callback(create(&jobs.last())); } RefPtr Execute::run(RefPtr shell) @@ -1150,7 +1126,7 @@ RefPtr IfCond::run(RefPtr shell) if (cond_job->signaled()) return create({}); // Exit early. - if (cond_job->exit_code() == 0) { + if (shell->last_return_code == 0) { if (m_true_branch) return m_true_branch->run(shell); } else { @@ -1287,23 +1263,9 @@ void Or::dump(int level) const RefPtr Or::run(RefPtr shell) { - - auto left = m_left->run(shell); - ASSERT(left->is_job()); - - auto* job_value = static_cast(left.ptr()); - const auto job = job_value->job(); - if (!job) { - // Something has gone wrong, let's just pretend that the job failed. - return m_right->run(shell); - } - - shell->block_on_job(job); - - if (job->exit_code() == 0) - return job_value; - - return m_right->run(shell); + auto commands = m_left->run(shell)->resolve_as_commands(shell); + commands.last().next_chain.empend(*m_right, NodeWithAction::Or); + return create(move(commands)); } void Or::highlight_in_editor(Line::Editor& editor, Shell& shell, HighlightMetadata metadata) @@ -1549,6 +1511,7 @@ RefPtr Sequence::run(RefPtr shell) execute_node = create(m_right->position(), m_right); return execute_node->run(shell); } + auto left = m_left->run(shell)->resolve_as_commands(shell); // This could happen if a comment is next to a command. if (left.size() == 1) { @@ -1557,13 +1520,12 @@ RefPtr Sequence::run(RefPtr shell) return m_right->run(shell); } - auto right = m_right->run(shell)->resolve_as_commands(shell); + if (left.last().should_wait) + left.last().next_chain.append(NodeWithAction { *m_right, NodeWithAction::Sequence }); + else + left.append(m_right->run(shell)->resolve_as_commands(shell)); - Vector commands; - commands.append(left); - commands.append(right); - - return create(move(commands)); + return create(move(left)); } void Sequence::highlight_in_editor(Line::Editor& editor, Shell& shell, HighlightMetadata metadata) diff --git a/Shell/AST.h b/Shell/AST.h index b4154a19903..ce8d7358b4d 100644 --- a/Shell/AST.h +++ b/Shell/AST.h @@ -28,6 +28,7 @@ #include "Forward.h" #include "Job.h" +#include #include #include #include @@ -174,13 +175,30 @@ public: pid_t pgid { -1 }; }; +struct NodeWithAction { + mutable NonnullRefPtr node; + enum Action { + And, + Or, + Sequence, + } action; + + NodeWithAction(Node& node, Action action) + : node(node) + , action(action) + { + } +}; + struct Command { Vector argv; NonnullRefPtrVector redirections; - mutable RefPtr pipeline; bool should_wait { true }; bool is_pipe_source { false }; bool should_notify_if_in_background { true }; + + mutable RefPtr pipeline; + Vector next_chain; }; struct HitTestResult { @@ -215,7 +233,7 @@ public: } CommandValue(Vector argv) - : m_command({ move(argv), {}, {}, true, false, true }) + : m_command({ move(argv), {}, true, false, true, nullptr, {} }) { } @@ -424,7 +442,6 @@ private: virtual void highlight_in_editor(Line::Editor&, Shell&, HighlightMetadata = {}) override; virtual HitTestResult hit_test_position(size_t) override; virtual String class_name() const override { return "And"; } - virtual bool would_execute() const override { return true; } RefPtr m_left; RefPtr m_right; @@ -458,7 +475,6 @@ private: virtual void highlight_in_editor(Line::Editor&, Shell&, HighlightMetadata = {}) override; virtual HitTestResult hit_test_position(size_t) override; virtual String class_name() const override { return "Background"; } - virtual bool would_execute() const override { return m_command->would_execute(); } RefPtr m_command; }; @@ -725,7 +741,6 @@ private: virtual void highlight_in_editor(Line::Editor&, Shell&, HighlightMetadata = {}) override; virtual HitTestResult hit_test_position(size_t) override; virtual String class_name() const override { return "Or"; } - virtual bool would_execute() const override { return true; } RefPtr m_left; RefPtr m_right; diff --git a/Shell/Forward.h b/Shell/Forward.h index 5cf82ce2f0c..bbe92ea8070 100644 --- a/Shell/Forward.h +++ b/Shell/Forward.h @@ -29,10 +29,11 @@ class Shell; namespace AST { +struct Command; class Node; class Value; class SyntaxError; class Pipeline; -class Rewiring; +struct Rewiring; } diff --git a/Shell/Job.cpp b/Shell/Job.cpp index 498fc850c17..8584695099f 100644 --- a/Shell/Job.cpp +++ b/Shell/Job.cpp @@ -26,6 +26,7 @@ #include "Job.h" #include "AST.h" +#include "Shell.h" #include #include #include @@ -70,3 +71,42 @@ bool Job::print_status(PrintStatusMode mode) return true; } + +Job::Job(pid_t pid, unsigned pgid, String cmd, u64 job_id, AST::Command&& command) + : m_pgid(pgid) + , m_pid(pid) + , m_job_id(job_id) + , m_cmd(move(cmd)) +{ + m_command = make(move(command)); + + set_running_in_background(false); + m_command_timer.start(); +} + +void Job::set_has_exit(int exit_code) +{ + if (m_exited) + return; + m_exit_code = exit_code; + m_exited = true; + if (on_exit) + on_exit(*this); +} + +void Job::set_signalled(int sig) +{ + if (m_exited) + return; + m_exited = true; + m_exit_code = 126; + m_term_sig = sig; + if (on_exit) + on_exit(*this); +} + +void Job::unblock() const +{ + if (!m_exited && on_exit) + on_exit(*this); +} diff --git a/Shell/Job.h b/Shell/Job.h index 394feef7b6d..9f89bf54b87 100644 --- a/Shell/Job.h +++ b/Shell/Job.h @@ -41,9 +41,11 @@ # undef JOB_TIME_INFO #endif +struct LocalFrame; + class Job : public RefCounted { public: - static NonnullRefPtr create(pid_t pid, pid_t pgid, String command, u64 job_id, AST::Pipeline* pipeline = nullptr) { return adopt(*new Job(pid, pgid, move(command), job_id, pipeline)); } + static NonnullRefPtr create(pid_t pid, pid_t pgid, String cmd, u64 job_id, AST::Command&& command) { return adopt(*new Job(pid, pgid, move(cmd), job_id, move(command))); } ~Job() { @@ -53,13 +55,15 @@ public: dbg() << "Command \"" << m_cmd << "\" finished in " << elapsed << " ms"; } #endif - if (m_pipeline) - m_pipeline = nullptr; } + Function)> on_exit; + pid_t pgid() const { return m_pgid; } pid_t pid() const { return m_pid; } const String& cmd() const { return m_cmd; } + const AST::Command& command() const { return *m_command; } + AST::Command* command_ptr() { return m_command; } u64 job_id() const { return m_job_id; } bool exited() const { return m_exited; } bool signaled() const { return m_term_sig != -1; } @@ -78,34 +82,12 @@ public: bool is_running_in_background() const { return m_running_in_background; } bool should_announce_exit() const { return m_should_announce_exit; } bool is_suspended() const { return m_is_suspended; } - void unblock() const - { - if (!m_exited && on_exit) - on_exit(*this); - } - Function)> on_exit; + void unblock() const; Core::ElapsedTimer& timer() { return m_command_timer; } - void set_has_exit(int exit_code) - { - if (m_exited) - return; - m_exit_code = exit_code; - m_exited = true; - if (on_exit) - on_exit(*this); - } - void set_signalled(int sig) - { - if (m_exited) - return; - m_exited = true; - m_exit_code = 126; - m_term_sig = sig; - if (on_exit) - on_exit(*this); - } + void set_has_exit(int exit_code); + void set_signalled(int sig); void set_is_suspended(bool value) const { m_is_suspended = value; } @@ -127,18 +109,7 @@ public: bool print_status(PrintStatusMode); private: - Job(pid_t pid, unsigned pgid, String cmd, u64 job_id, AST::Pipeline* pipeline) - : m_pgid(pgid) - , m_pid(pid) - , m_job_id(job_id) - , m_cmd(move(cmd)) - { - if (pipeline) - m_pipeline = *pipeline; - - set_running_in_background(false); - m_command_timer.start(); - } + Job(pid_t pid, unsigned pgid, String cmd, u64 job_id, AST::Command&& command); pid_t m_pgid { 0 }; pid_t m_pid { 0 }; @@ -153,5 +124,5 @@ private: mutable bool m_active { true }; mutable bool m_is_suspended { false }; bool m_should_be_disowned { false }; - RefPtr m_pipeline; + OwnPtr m_command; }; diff --git a/Shell/Parser.cpp b/Shell/Parser.cpp index 4d4b3276aeb..650a78cd8d7 100644 --- a/Shell/Parser.cpp +++ b/Shell/Parser.cpp @@ -283,7 +283,7 @@ RefPtr Parser::parse_or_logical_sequence() if (!right_and_sequence) right_and_sequence = create("Expected an expression after '||'"); - return create(create(move(and_sequence)), create(move(right_and_sequence))); + return create(move(and_sequence), move(right_and_sequence)); } RefPtr Parser::parse_and_logical_sequence() @@ -305,7 +305,7 @@ RefPtr Parser::parse_and_logical_sequence() if (!right_and_sequence) right_and_sequence = create("Expected an expression after '&&'"); - return create(create(move(pipe_sequence)), create(move(right_and_sequence))); + return create(move(pipe_sequence), move(right_and_sequence)); } RefPtr Parser::parse_pipe_sequence() diff --git a/Shell/Shell.cpp b/Shell/Shell.cpp index 4e14d7bad51..77a14594457 100644 --- a/Shell/Shell.cpp +++ b/Shell/Shell.cpp @@ -421,15 +421,7 @@ int Shell::run_command(const StringView& cmd) tcgetattr(0, &termios); - auto result = command->run(*this); - if (result->is_job()) { - auto job_result = static_cast(result.ptr()); - auto job = job_result->job(); - if (!job) - last_return_code = 0; - else if (job->exited()) - last_return_code = job->exit_code(); - } + command->run(*this); return last_return_code; } @@ -500,8 +492,11 @@ RefPtr Shell::run_command(const AST::Command& command) } int retval = 0; - if (run_builtin(command, rewirings, retval)) + if (run_builtin(command, rewirings, retval)) { + for (auto& next_in_chain : command.next_chain) + run_tail(next_in_chain, retval); return nullptr; + } Vector argv; Vector copy_argv = command.argv; @@ -619,15 +614,19 @@ RefPtr Shell::run_command(const AST::Command& command) StringBuilder cmd; cmd.join(" ", command.argv); - auto job = Job::create(child, pgid, cmd.build(), find_last_job_id() + 1, command.pipeline); + auto job = Job::create(child, pgid, cmd.build(), find_last_job_id() + 1, AST::Command(command)); jobs.set((u64)child, job); - job->on_exit = [](auto job) { + job->on_exit = [this](auto job) { if (!job->exited()) return; if (job->is_running_in_background() && job->should_announce_exit()) fprintf(stderr, "Shell: Job %" PRIu64 "(%s) exited\n", job->job_id(), job->cmd().characters()); + + last_return_code = job->exit_code(); job->disown(); + + run_tail(job); }; fds.collect(); @@ -635,9 +634,42 @@ RefPtr Shell::run_command(const AST::Command& command) return *job; } +void Shell::run_tail(const AST::NodeWithAction& next_in_chain, int head_exit_code) +{ + switch (next_in_chain.action) { + case AST::NodeWithAction::And: + if (head_exit_code == 0) { + auto commands = next_in_chain.node->run(*this)->resolve_as_commands(*this); + run_commands(commands); + } + break; + case AST::NodeWithAction::Or: + if (head_exit_code != 0) { + auto commands = next_in_chain.node->run(*this)->resolve_as_commands(*this); + run_commands(commands); + } + break; + case AST::NodeWithAction::Sequence: + auto commands = next_in_chain.node->run(*this)->resolve_as_commands(*this); + run_commands(commands); + break; + } +} + +void Shell::run_tail(RefPtr job) +{ + if (auto cmd = job->command_ptr()) { + deferred_invoke([=, this](auto&) { + for (auto& next_in_chain : cmd->next_chain) { + run_tail(next_in_chain, job->exit_code()); + } + }); + } +} + NonnullRefPtrVector Shell::run_commands(Vector& commands) { - NonnullRefPtrVector jobs_to_wait_for; + NonnullRefPtrVector spawned_jobs; for (auto& command : commands) { #ifdef SH_DEBUG @@ -663,21 +695,19 @@ NonnullRefPtrVector Shell::run_commands(Vector& commands) if (!job) continue; + spawned_jobs.append(*job); if (command.should_wait) { block_on_job(job); - if (!job->is_suspended()) - jobs_to_wait_for.append(*job); } else { if (command.is_pipe_source) { job->set_running_in_background(true); - jobs_to_wait_for.append(*job); } else if (command.should_notify_if_in_background) { job->set_should_announce_exit(true); } } } - return jobs_to_wait_for; + return spawned_jobs; } bool Shell::run_file(const String& filename, bool explicitly_invoked) @@ -709,6 +739,9 @@ void Shell::block_on_job(RefPtr job) if (!job) return; + if (job->is_suspended()) + return; // We cannot wait for a suspended job. + ScopeGuard io_restorer { [&]() { if (job->exited() && !job->is_running_in_background()) { restore_ios(); diff --git a/Shell/Shell.h b/Shell/Shell.h index c842bbc28b8..9376800bd2a 100644 --- a/Shell/Shell.h +++ b/Shell/Shell.h @@ -195,6 +195,9 @@ private: const Job* m_current_job { nullptr }; LocalFrame* find_frame_containing_local_variable(const String& name); + void run_tail(RefPtr); + void run_tail(const AST::NodeWithAction&, int head_exit_code); + virtual void custom_event(Core::CustomEvent&) override; #define __ENUMERATE_SHELL_BUILTIN(builtin) \ diff --git a/Shell/Tests/if.sh b/Shell/Tests/if.sh index 342579da26c..598a9471bc0 100644 --- a/Shell/Tests/if.sh +++ b/Shell/Tests/if.sh @@ -1,13 +1,17 @@ #!/bin/sh +setopt --verbose + if test 1 -eq 1 { # Are comments ok? # Basic 'if' structure, empty block. if true { } else { + echo "if true runs false branch" exit 2 } if false { + echo "if false runs true branch" exit 2 } else { } diff --git a/Shell/Tests/valid.sh b/Shell/Tests/valid.sh index 6453d5f21b0..c6a49182023 100644 --- a/Shell/Tests/valid.sh +++ b/Shell/Tests/valid.sh @@ -23,69 +23,69 @@ test "$(echo yes)" = yes || exit 2 alias fail="echo Failure: " # Redirections. -test -z "$(echo foo > /dev/null)" || fail direct path redirection -test -z "$(echo foo 2> /dev/null 1>&2)" || fail indirect redirection -test -n "$(echo foo 2> /dev/null)" || fail fds interfere with each other +test -z "$(echo foo > /dev/null)" || fail direct path redirection && exit 2 +test -z "$(echo foo 2> /dev/null 1>&2)" || fail indirect redirection && exit 2 +test -n "$(echo foo 2> /dev/null)" || fail fds interfere with each other && exit 2 # Argument unpack -test "$(echo (yes))" = yes || fail arguments inside bare lists -test "$(echo (no)() yes)" = yes || fail arguments inside juxtaposition: empty -test "$(echo (y)(es))" = yes || fail arguments inside juxtaposition: list -test "$(echo "y"es)" = yes || fail arguments inside juxtaposition: string +test "$(echo (yes))" = yes || fail arguments inside bare lists && exit 2 +test "$(echo (no)() yes)" = yes || fail arguments inside juxtaposition: empty && exit 2 +test "$(echo (y)(es))" = yes || fail arguments inside juxtaposition: list && exit 2 +test "$(echo "y"es)" = yes || fail arguments inside juxtaposition: string && exit 2 # String substitution foo=yes -test "$(echo $foo)" = yes || fail simple string var lookup -test "$(echo "$foo")" = yes || fail stringified string var lookup +test "$(echo $foo)" = yes || fail simple string var lookup && exit 2 +test "$(echo "$foo")" = yes || fail stringified string var lookup && exit 2 # List substitution foo=(yes) # Static lookup, as list -test "$(echo $foo)" = yes || fail simple list var lookup +test "$(echo $foo)" = yes || fail simple list var lookup && exit 2 # Static lookup, stringified -test "$(echo "$foo")" = yes || fail stringified list var lookup +test "$(echo "$foo")" = yes || fail stringified list var lookup && exit 2 # Dynamic lookup through static expression -test "$(echo $'foo')" = yes || fail dynamic lookup through static exp +test "$(echo $'foo')" = yes || fail dynamic lookup through static exp && exit 2 # Dynamic lookup through dynamic expression ref_to_foo=foo -test "$(echo $"$ref_to_foo")" = yes || fail dynamic lookup through dynamic exp +test "$(echo $"$ref_to_foo")" = yes || fail dynamic lookup through dynamic exp && exit 2 # More redirections echo test > /tmp/sh-test -test "$(cat /tmp/sh-test)" = test || fail simple path redirect +test "$(cat /tmp/sh-test)" = test || fail simple path redirect && exit 2 rm /tmp/sh-test # 'brace' expansions -test "$(echo x(yes no))" = "xyes xno" || fail simple juxtaposition expansion -test "$(echo (y n)(es o))" = "yes yo nes no" || fail list-list juxtaposition expansion -test "$(echo ()(foo bar baz))" = "" || fail empty expansion +test "$(echo x(yes no))" = "xyes xno" || fail simple juxtaposition expansion && exit 2 +test "$(echo (y n)(es o))" = "yes yo nes no" || fail list-list juxtaposition expansion && exit 2 +test "$(echo ()(foo bar baz))" = "" || fail empty expansion && exit 2 # Variables inside commands to_devnull=(>/dev/null) -test "$(echo hewwo $to_devnull)" = "" || fail variable containing simple command +test "$(echo hewwo $to_devnull)" = "" || fail variable containing simple command && exit 2 word_count=(() | wc -w) -test "$(echo well hello friends $word_count)" -eq 3 || fail variable containing pipeline +test "$(echo well hello friends $word_count)" -eq 3 || fail variable containing pipeline && exit 2 # Globs mkdir sh-test pushd sh-test touch (a b c)(d e f) - test "$(echo a*)" = "ad ae af" || fail '*' glob expansion - test "$(echo a?)" = "ad ae af" || fail '?' glob expansion + test "$(echo a*)" = "ad ae af" || fail '*' glob expansion && exit 2 + test "$(echo a?)" = "ad ae af" || fail '?' glob expansion && exit 2 glob_in_var='*' - test "$(echo $glob_in_var)" = '*' || fail substituted string acts as glob + test "$(echo $glob_in_var)" = '*' || fail substituted string acts as glob && exit 2 - test "$(echo (a*))" = "ad ae af" || fail globs in lists resolve wrong - test "$(echo x(a*))" = "xad xae xaf" || fail globs in lists do not resolve to lists - test "$(echo "foo"a*)" = "fooad fooae fooaf" || fail globs join to dquoted strings + test "$(echo (a*))" = "ad ae af" || fail globs in lists resolve wrong && exit 2 + test "$(echo x(a*))" = "xad xae xaf" || fail globs in lists do not resolve to lists && exit 2 + test "$(echo "foo"a*)" = "fooad fooae fooaf" || fail globs join to dquoted strings && exit 2 popd rm -fr sh-test # Setopt setopt --inline_exec_keep_empty_segments -test "$(echo -n "a\n\nb")" = "a b" || fail inline_exec_keep_empty_segments has no effect +test "$(echo -n "a\n\nb")" = "a b" || fail inline_exec_keep_empty_segments has no effect && exit 2 setopt --no_inline_exec_keep_empty_segments -test "$(echo -n "a\n\nb")" = "a b" || fail cannot unset inline_exec_keep_empty_segments +test "$(echo -n "a\n\nb")" = "a b" || fail cannot unset inline_exec_keep_empty_segments && exit 2