Merge pull request #5178 from BrianNixon/manager_sort_requests

[Manager] Sort requests to suspend/resume/abort multiple tasks

clientgui/MainDocument.cpp

@@ -1,6 +1,6 @@
 // This file is part of BOINC.
 // http://boinc.berkeley.edu
-// Copyright (C) 2022 University of California
+// Copyright (C) 2023 University of California
 //
 // BOINC is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License
@@ -1561,7 +1561,7 @@ int CMainDocument::GetWorkCount() {
 }
 
 
-int CMainDocument::WorkSuspend(char* url, char* name) {
+int CMainDocument::WorkSuspend(const char* url, const char* name) {
     int iRetVal = 0;
 
     RESULT* pStateResult = state.lookup_result(url, name);
@@ -1575,7 +1575,7 @@ int CMainDocument::WorkSuspend(char* url, char* name) {
 }
 
 
-int CMainDocument::WorkResume(char* url, char* name) {
+int CMainDocument::WorkResume(const char* url, const char* name) {
     int iRetVal = 0;
 
     RESULT* pStateResult = state.lookup_result(url, name);
@@ -1984,7 +1984,7 @@ int CMainDocument::WorkShowVMConsole(RESULT* res) {
 }
 
 
-int CMainDocument::WorkAbort(char* url, char* name) {
+int CMainDocument::WorkAbort(const char* url, const char* name) {
     int iRetVal = 0;
 
     RESULT* pStateResult = state.lookup_result(url, name);

clientgui/MainDocument.h

@@ -1,6 +1,6 @@
 // This file is part of BOINC.
 // http://boinc.berkeley.edu
-// Copyright (C) 2022 University of California
+// Copyright (C) 2023 University of California
 //
 // BOINC is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License
@@ -279,11 +279,11 @@ public:
 
     int                         GetWorkCount();
 
-    int                         WorkSuspend(char* url, char* name);
-    int                         WorkResume(char* url, char* name);
+    int                         WorkSuspend(const char* url, const char* name);
+    int                         WorkResume(const char* url, const char* name);
     int                         WorkShowGraphics(RESULT* result);
     int                         WorkShowVMConsole(RESULT* result);
-    int                         WorkAbort(char* url, char* name);
+    int                         WorkAbort(const char* url, const char* name);
     CC_STATE*                   GetState() { return &state; };
 
 

clientgui/ViewWork.cpp

@@ -1,6 +1,6 @@
 // This file is part of BOINC.
 // http://boinc.berkeley.edu
-// Copyright (C) 2022 University of California
+// Copyright (C) 2023 University of California
 //
 // BOINC is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License
@@ -462,6 +462,15 @@ void CViewWork::OnActiveTasksOnly( wxCommandEvent& WXUNUSED(event) ) {
 }
 
 
+// Comparator functions for sort operations in OnWorkSuspend() and OnWorkAbort()
+static bool sort_order_started(const RESULT* a, const RESULT* b) {
+    return !a->is_not_started() && b->is_not_started();
+}
+static bool sort_order_not_started(const RESULT* a, const RESULT* b) {
+    return a->is_not_started() && !b->is_not_started();
+}
+
+
 void CViewWork::OnWorkSuspend( wxCommandEvent& WXUNUSED(event) ) {
     wxLogTrace(wxT("Function Start/End"), wxT("CViewWork::OnWorkSuspend - Function Begin"));
 
@@ -476,6 +485,13 @@ void CViewWork::OnWorkSuspend( wxCommandEvent& WXUNUSED(event) ) {
     wxASSERT(m_pTaskPane);
     wxASSERT(m_pListPane);
 
+    // It shouldn't be possible to get in here with a mixture of suspended and non-
+    // suspended tasks selected, so one of these lists should always end up empty.
+    // It is not possible for any one task to end up in both lists.
+    std::vector<RESULT*> results_to_resume;
+    std::vector<RESULT*> results_to_suspend;
+
+    // Collect the selected tasks, without actioning the request yet
     row = -1;
     while (1) {
         // Step through all selected items
@@ -485,13 +501,41 @@ void CViewWork::OnWorkSuspend( wxCommandEvent& WXUNUSED(event) ) {
         RESULT* result = pDoc->result(m_iSortedIndexes[row]);
         if (result) {
             if (result->suspended_via_gui) {
-                pDoc->WorkResume(result->project_url, result->name);
+                results_to_resume.push_back(result);
             } else {
-                pDoc->WorkSuspend(result->project_url, result->name);
+                results_to_suspend.push_back(result);
             }
         }
     }
 
+    // Sort
+    //
+    // Until/unless the client can accept requests to operate on a batch of tasks at once
+    // (and thus make its own decision about the order in which to perform the actions),
+    // sort the lists to avoid unwanted side-effects of the client asynchronously scheduling tasks.
+    // In particular this takes care of the case where the user wants to suspend all tasks,
+    // some of which are running and some of which have not yet started. Without this sorting,
+    // suspending the first running task will open up a slot to the client, which will immediately
+    // find a ready task and start it running, not knowing that we also want to suspend that one.
+    // Resume is the opposite: resume tasks that have already started before those that have not.
+    // Otherwise, the user probably expects to see requests take effect in the same order as the
+    // selection in the GUI - so use a stable sort.
+    std::stable_sort(results_to_resume.begin(), results_to_resume.end(), sort_order_started);
+    std::stable_sort(results_to_suspend.begin(), results_to_suspend.end(), sort_order_not_started);
+
+    // Do the action(s)
+    //
+    // One of these lists should always be empty, so the order in which we action
+    // the two makes no difference. If somehow we do end up with entries in both,
+    // sending the resume requests first gives the client a more up-to-date picture
+    // before the suspensions cause it to go looking for tasks that are ready to run.
+    for (const RESULT* result : results_to_resume) {
+        pDoc->WorkResume(result->project_url, result->name);
+    }
+    for (const RESULT* result : results_to_suspend) {
+        pDoc->WorkSuspend(result->project_url, result->name);
+    }
+
     UpdateSelection();
     pFrame->FireRefreshView();
 
@@ -615,6 +659,8 @@ void CViewWork::OnWorkAbort( wxCommandEvent& WXUNUSED(event) ) {
         return;
     }
 
+    std::vector<RESULT*> results_to_abort;
+
     row = -1;
     while (1) {
         // Step through all selected items
@@ -623,10 +669,18 @@ void CViewWork::OnWorkAbort( wxCommandEvent& WXUNUSED(event) ) {
         
         RESULT* result = pDoc->result(m_iSortedIndexes[row]);
         if (result) {
-            pDoc->WorkAbort(result->project_url, result->name);
+            results_to_abort.push_back(result);
         }
     }
 
+    // Abort tasks that have not yet started before those that have.
+    // See longer explanation in OnWorkSuspend()
+    std::stable_sort(results_to_abort.begin(), results_to_abort.end(), sort_order_not_started);
+
+    for (const RESULT* result : results_to_abort) {
+        pDoc->WorkAbort(result->project_url, result->name);
+    }
+
     UpdateSelection();
     pFrame->FireRefreshView();
 

lib/gui_rpc_client.h

@@ -1,6 +1,6 @@
 // This file is part of BOINC.
 // https://boinc.berkeley.edu
-// Copyright (C) 2022 University of California
+// Copyright (C) 2023 University of California
 //
 // BOINC is free software; you can redistribute it and/or modify it
 // under the terms of the GNU Lesser General Public License
@@ -311,6 +311,13 @@ struct RESULT {
     int parse(XML_PARSER&);
     void print();
     void clear();
+
+    bool is_not_started() const {
+        if (state >= RESULT_COMPUTE_ERROR) return false;
+        if (ready_to_report) return false;
+        if (active_task) return false;
+        return true;
+    }
 };
 
 struct FILE_TRANSFER {