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

Dont suppress StopUser or GreenletExit in on_stop #2486

Conversation

ALagierski
Copy link
Contributor

The #2402 introduced change which wraps on_stop call around try/except block inside handling of InterruptTaskSet exception.

The initial implementation assumed that every exception shall be handled and logged, without any further consequences.
This appeared to be a little bit too greedy. It allowed the execution to carry on.

When running our unit tests we observed a situation where the execution falls into infinite loop. This was because our on_stop method implementation counted its invocation and was supposed to raise StopUser exception after reaching some arbitrarly chosen number.

We do believe that this behavior is inappropriate, as StopUser is supposed to always stop execution of current user.

We wanted to propose a solution for this issue presented in this Pull Request.

Attached unit test is a simple copy of our unit test. The purpose of this unit test is to verify whether interrupts in SequentialTaskSet truly reschedule the execution of whole taskset. If interrupt occurs before calling all tasks, tasks defined after the interrupt shall never be executed. This was verified by the counter in our test. In order to exit the loop, we deliberately raise StopUser in on_stop method.

* Add exception handling for a group of `StopUser` &
  `GreenletExit` exceptions which can be raised in `on_stop` method in
  handling of `InterruptTaskSet` exception inside `run` method. When
  either of those exceptions are raised, the execution shall be stopped.
* Add unit test to cover this scenario.
@cyberw
Copy link
Collaborator

cyberw commented Nov 24, 2023

Looks good! I still dont really like TaskSets :)

@cyberw cyberw merged commit 32b1bba into locustio:master Nov 24, 2023
13 of 15 checks passed
@cyberw cyberw changed the title Apply patch for handling InterruptTaskSet exception Dont suppress StopUser or GreenletExit in on_stop Nov 24, 2023
@domik82
Copy link

domik82 commented Nov 24, 2023

@cyberw - we use it but some of the implementations are just old while we were still learning how to use Locust. Some are not perfect.

For example this fail showed us that our implementation of reschedule is actually wrong. We had an assumption that reschedule=False causes the task will not be executed anymore. This is not true - task will be rescheduled no matter what you will put there. It will be just done right away or 'a bit later' while our assumption was that it will be 'never' if set to False ;). The naming of this param is just - not intuitive to say the least :)

Good news for us is that we had this bug since 2021 and it would only manifest in case of failures so we can live with it for now.

Probably there should be some work done to refactor naming a bit. Like instantly_reschedule=True/False or something along those lines - this could be breaking change for some so it's not that straightforward.

On the other side, reschedule=False should actually stop that task from running. So to keep old behavior it would have to have param like - retry=True/False

Those two could work together retry=True + instantly_reschedule=True

I'm not sure how much code changes that would be.

@cyberw
Copy link
Collaborator

cyberw commented Nov 24, 2023

All these things just emphasize why TaskSets are such a bad idea. In other tools where tests are not expressed as code a TaskSet concept might make sense, but not in Locust. These bugs/oddities/unexpected behavior is just another example of that :)

I'm sure things can be fixed, but imo it is just the wrong way to go. Explicit, regular "native" python programming concepts are much more powerful and come with no surprises.

@cyberw
Copy link
Collaborator

cyberw commented Nov 25, 2023

Hmm. The build is failing due to a typing issue with python 3.8. (I merged it thinking the build was just flaky) Can you have a look?

@domik82
Copy link

domik82 commented Nov 25, 2023

We will take a look. I also thought it's just flaky test. Not sure if we have any ability to run pipeline on our own. We use python 3.11 so there is no way to catch it earlier.

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

Successfully merging this pull request may close these issues.

3 participants