Start web_ui later to avoid race adding UI routes

[solowalker27]

Flask throws an exception AssertionError: A setup function was called after the first request was handled… if it serves a request before all the routes are added, creating a race condition. Starting the web UI after firing init event avoids this race.

Requires web_ui.start() to be called manually when create_web_ui() or WebUI() are called or instantiated manually.

Technically this is a bit of a breaking change because anyone currently relying on create_web_ui() or WebUI() to immediately start the web UI will have to add one more call afterward to do so. I'm interested in feedback about this. In one small way, it's still consistent with how the rest of Locust works since runner.start() is required even after instantiation before it does anything.

Thoughts?

[cyberw]

Hmm.. I'm not a huge fan of this change as it does complicate things a little for users of the api. Shouldnt it be really unlikely for someone to have time to execute a request before init?

[cyberw]

Perhaps we can just block flask from serving requests until init has been run?

[solowalker27]

In theory it should be unlikely. In practice, it's quite possible. My coworkers hit it frequently in situations such as leaving a tab open with the web UI showing results from a previous run, stopping Locust, starting it again, and then attempting to open another tab to start and control a new test. The old tab seems to continue to attempt to communicate with Locust UI even after it's torn down and if it happens to get a successful request in at the right moment when Locust is starting again, this happens.

We also use Locust as part of a larger set of tools that includes a status page about whether or not the UI is up and ready to be used. That service periodically pings the / endpoint to see if is available. The same thing can occur with this.

I'm open to other ideas. Do you know of a way to block Flask from serving requests until we tell it to even if it's been started? Functionally, I'm not sure what the difference would be if there's still some other function that needs to be called before the UI is ready. I thought about adding an artificial delay to give the init time to complete first, but that felt hacky and may not necessarily solve the issue 100% as this does.

[cyberw]

I see.

Maybe you can launch the greenlet using an init listener defined in locust? or would it be triggered before user defined events? (I dont know the order they are fired)

[codecov]

Codecov Report

Merging #1585 into master will decrease coverage by 0.19%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master    #1585      +/-   ##
==========================================
- Coverage   82.08%   81.89%   -0.20%     
==========================================
  Files          28       28              
  Lines        2596     2601       +5     
  Branches      395      397       +2     
==========================================
- Hits         2131     2130       -1     
- Misses        367      373       +6     
  Partials       98       98              

Impacted Files	Coverage Δ
locust/env.py	96.82% <ø> (ø)
locust/main.py	20.00% <0.00%> (-0.18%)	⬇️
locust/web.py	91.32% <80.00%> (-1.15%)	⬇️
locust/runners.py	83.05% <0.00%> (-0.37%)	⬇️
locust/stats.py	90.08% <0.00%> (+0.20%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32de954...410cf8b. Read the comment docs.

[solowalker27]

With this new approach, web_ui.start() is no longer required to be called. delayed_start added as optional parameter to WebUI and create_web_ui() that gates starting web UI immediately or after init event is fired.

Only downside I can think of is if you're starting the web UI manually via create_web_ui() and you want to add some routes but avoid the race condition, you'd have to pass in delayed_start=True, add your routes, and then manually fire the init event. Perhaps this is enough of an edge case that that might be acceptable?

[cyberw]

I think the new solution is good, or at least as good as can be done without doing a major rewrite :)

Can you remove the commented out line?

[cyberw]

Or actually, maybe the old solution is more robust & clean. Using events is kind of a heavy solution, and when it doesnt solve 100% of the cases it probably isnt worth it. Which one do you prefer?

[solowalker27]

Solution 3. It's a bit of a combination of the two previous ones. Still uses delayed_start but doesn't use events. Is this better? Now instead, when anyone wants to use delayed_start they just call web_ui.start() directly. I don't know what exactly makes events a "heavy solution", but this sidesteps that with the same result.

[cyberw]

I think the new solution looks nice. The reason it felt ”heavy” was because I dont think we use events internally (and it shouldnt really be necessary, it is just an integration point for external code).

One last thing, can you remove the **kwargs argument?

[solowalker27]

Hmmm. I've run all tests multiple times following my changes and I haven't hit that error Travis did. All mine are passing clean.

[cyberw]

Looks like flakyness, nothing to worry about. Thanks!