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

Stop workflow doesn't clean up the task queue #573

Open
ZihengSun opened this issue Oct 23, 2024 · 4 comments
Open

Stop workflow doesn't clean up the task queue #573

ZihengSun opened this issue Oct 23, 2024 · 4 comments

Comments

@ZihengSun
Copy link
Member

If you hit stop button in Weaver, the rest of the tasks will still be triggered. They should all be cleaned up. The ongoing tasks should be stopped instantly.

@saikiranAnnam
Copy link

saikiranAnnam commented Nov 2, 2024

To address the issue (#573 ) where workflows with only two processes fail to execute properly, we can adjust the notifyWaitinglist method to handle cases with minimal waiting list sizes more effectively.

Explanation of changes :

  1. Using an iterator we can safely modify the waiting list while looping(i.e. traversing), preventing ConcurrentModificationException.

  2. After processing a task (either by ending it prematurely or executing it), call the notifyWaitinglist() again. This way, If there are any still pending tasks, they can be processed immediately.

  3. Breaking the loop after each task is processed ensures the tasks are handled individually, which is particularly helpful for workflows with only two processes (issue: Stop workflow doesn't clean up the task queue #573) or minimal waiting lists.

Proposal Logic Fixture:

  1. Use an iterator instead of a for loop to iterate through the waiting list. This avoids issues arising from modifying the list while looping over it.

  2. After removing or ending a task, explicitly break out of the loop to re-trigger the notifyWaitinglist() method. This ensures each task is processed one at a time, even in scenarios with only two tasks.

Code Adjustments:

src/main/java/com/gw/tasks/TaskManager.java

public synchronized void notifyWaitinglist() {
    logger.debug("notify waiting list to pay attention to the released worker");
    if (waitinglist.size() > 0 && wm.getCurrentWorkerNumber() < worknumber) {
        
       orderWaitingList(); // Ensure highest-priority tasks are first

        Iterator<Task> iterator = waitinglist.iterator();
        while (iterator.hasNext()) {
            GeoweaverProcessTask newtask = (GeoweaverProcessTask) iterator.next();

            if (newtask.getIsReady()) {
                iterator.remove(); // Safely remove task using iterator

                if (newtask.checkShouldPassOrNot()) {
                    newtask.endPrematurely();
                    notifyWaitinglist(); // Recursively call to handle remaining tasks
                    break; // Break to prevent ConcurrentModificationException
                } else {
                    executeATask(newtask);
                    break; // Exit to allow notifyWaitinglist to run again
                }
            }
        }
    }
}

These logic adjustments and proposal fixtures ensure that each task is given attention immediately after another finish. When only two processes are in the list, this ensures that both are processed in sequence without skipping or interference due to concurrent modifications.

Note: This is a proposed fix to address the issue. I am open to suggestions and have not yet tested it.

@srinijammula
Copy link
Collaborator

Tested the fix, but encountered an error while running the workflow. It appears that the flow isn't triggered and stops immediately.

image

@saikiranAnnam
Copy link

Is the old function (i.e notify function before this logic proposal) still working properly, or does it continue to show the same error related to credentials or host configuration? The error indicates a credentials issue or a host configuration problem.

@srinijammula
Copy link
Collaborator

Yeah the old function was able to run the workflow and doesn't give any log on error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants