From 7f5e58ae69759dc1f930cd26500f3df8cb1716a7 Mon Sep 17 00:00:00 2001
From: Mahmoud Al-Qudsi <mqudsi@neosmart.net>
Date: Thu, 28 Mar 2019 18:16:22 -0500
Subject: [PATCH] Only send JOB_EXIT after the job has been actually erased

---
 src/proc.cpp | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/src/proc.cpp b/src/proc.cpp
index 81584133b..b1bb87431 100644
--- a/src/proc.cpp
+++ b/src/proc.cpp
@@ -461,7 +461,7 @@ static bool process_clean_after_marking(bool allow_interactive) {
     ASSERT_IS_MAIN_THREAD();
     bool found = false;
 
-    // this function may fire an event handler, we do not want to call ourselves recursively (to
+    // This function may fire an event handler, we do not want to call ourselves recursively (to
     // avoid infinite recursion).
     static bool locked = false;
     if (locked) {
@@ -469,11 +469,13 @@ static bool process_clean_after_marking(bool allow_interactive) {
     }
     locked = true;
 
-    // this may be invoked in an exit handler, after the TERM has been torn down
-    // don't try to print in that case (#3222)
+    // This may be invoked in an exit handler, after the TERM has been torn down
+    // Don't try to print in that case (#3222)
     const bool interactive = allow_interactive && cur_term != NULL;
 
-    static std::vector<job_t *> erase_list;
+    // If we ever drop the `static bool locked` above, this should be changed to a local or
+    // thread-local vector instead of a static vector. It is only static to reduce heap allocations.
+    static std::vector<shared_ptr<job_t>> erase_list;
     const bool only_one_job = jobs().size() == 1;
     for (const auto &j : jobs()) {
         // If we are reaping only jobs who do not need status messages sent to the console, do not
@@ -565,9 +567,8 @@ static bool process_clean_after_marking(bool allow_interactive) {
             if (j->pgid != INVALID_PID) {
                 proc_fire_event(L"JOB_EXIT", event_type_t::exit, -j->pgid, 0);
             }
-            proc_fire_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id, 0);
 
-            erase_list.push_back(j.get());
+            erase_list.push_back(j);
         } else if (j->is_stopped() && !j->get_flag(job_flag_t::NOTIFIED)) {
             // Notify the user about newly stopped jobs.
             if (!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) {
@@ -585,10 +586,7 @@ static bool process_clean_after_marking(bool allow_interactive) {
         auto to_erase = erase_list.begin();
         jobs().erase(std::remove_if(jobs().begin(), jobs().end(),
                     [&to_erase](const shared_ptr<job_t> &j) {
-            if (to_erase == erase_list.end()) {
-                return false;
-            }
-            if (*to_erase == j.get()) {
+            if (to_erase != erase_list.end() && *to_erase == j) {
                 ++to_erase;
                 return true;
             }
@@ -606,6 +604,12 @@ static bool process_clean_after_marking(bool allow_interactive) {
                 && "Not all jobs slated for erasure have been erased!");
         }
     }
+
+    // Only now notify any listeners of the job exit, so that the contract isn't violated
+    for (const auto &j : erase_list) {
+        proc_fire_event(L"JOB_EXIT", event_type_t::job_exit, j->job_id, 0);
+    }
+
     erase_list.clear();
 
     if (found) {