-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Decouple Runner and Locust code by introducing Locust.start and Locust.stop methods #1306
Conversation
…t.stop methods. The start() and stop() methods takes a gevent.pool.Group instance, and is responsible for spawning and killing a greenlet running the Locust instance's run() method. With this change the runner does no longer need to keep track of the state of the individual Locust instances.
…CUST_STATE_STOPPING if TaskSet.interrupt() were called with reschedule argument set to False.
Codecov Report
@@ Coverage Diff @@
## master #1306 +/- ##
==========================================
+ Coverage 79.82% 80.21% +0.38%
==========================================
Files 23 23
Lines 2112 2118 +6
Branches 327 321 -6
==========================================
+ Hits 1686 1699 +13
+ Misses 341 339 -2
+ Partials 85 80 -5
Continue to review full report at Codecov.
|
…ntial race condition from preventing locust user to die
locust/core.py
Outdated
task_set_instance = self._task_set(self) | ||
try: | ||
if hasattr(self, "on_start"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be possible to define an on_start/on_stop method on the TaskSet base class that does nothing? Instead of checking whether they exist...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that might be better. I guess that any performance difference should be negligible (haven't measure so I don't even know which one is fastest).
locust/core.py
Outdated
""" | ||
def run_locust(user): | ||
""" | ||
Main function gor Locust user greenlet. It's important that this function takes the locust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gor? :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, will fix :)
else: | ||
locust._state = LOCUST_STATE_STOPPING | ||
dying.add(g) | ||
for user in users: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you're using "user/s" instead of "locust/s" because we're renaming it? it is confusing when the comments talk about locusts but the code about users :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I figured we're renaming it, and even before we've done that I don't think user
can be mistaken for anything else in that context.
Big thumbs up, apart from the minor questions/issues I mentioned. |
…be implemented by end-user test scripts. Fix bug where the TaskSet.on_stop method weren't called for nested TaskSets (only called for the first TaskSet immediately under the Locust). Should fix #1206.
I'm ok to merge! |
Decouples the runner code from the Locust user code by introducing Locust.start and Locust.stop methods.
The start() and stop() methods takes a gevent.pool.Group instance, and is responsible for spawning and killing a greenlet for the locust user. With this change the runner does no longer need to keep track of the state of the individual Locust instances, and the Locust instance does no longer need to be given a reference to the runner.
(This PR is based on the tasks-directly-under-locust-class branch, and shouldn't be merged until that PR is merged).