Skip to content

fix: prevent race condition when canceling query#11449

Merged
betodealmeida merged 3 commits into
apache:masterfrom
betodealmeida:SO-1056
Oct 29, 2020
Merged

fix: prevent race condition when canceling query#11449
betodealmeida merged 3 commits into
apache:masterfrom
betodealmeida:SO-1056

Conversation

@betodealmeida
Copy link
Copy Markdown
Member

@betodealmeida betodealmeida commented Oct 27, 2020

SUMMARY

With the right timing, it's possible to cancel a query, see the message and toast saying "Query was stopped", but still have results being loaded in SQL Lab shortly after. I added a check to prevent this from happening.

Note that actions.QUERY_FAILED has a similar check.

I also noticed that this has been fixed in the past (#2164), but removed later (#4833). @graceguo-supercat and @john-bodley do you have context why it was removed? It doesn't seem obvious to me from the PR.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TEST PLAN

The problem is hard to repro because of the sensitive timing, but I was able to make it happen twice by changing the examples database to async and canceling running queries at the right time.

After the fix was applied I wasn't able to repro the problem.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@codecov-io
Copy link
Copy Markdown

codecov-io commented Oct 27, 2020

Codecov Report

Merging #11449 into master will decrease coverage by 0.69%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #11449      +/-   ##
==========================================
- Coverage   62.26%   61.57%   -0.70%     
==========================================
  Files         863      417     -446     
  Lines       40978    25960   -15018     
  Branches     3693        0    -3693     
==========================================
- Hits        25516    15984    -9532     
+ Misses      15283     9976    -5307     
+ Partials      179        0     -179     
Flag Coverage Δ
#javascript ?
#python 61.57% <ø> (-0.35%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
superset/db_engine_specs/mysql.py 79.59% <0.00%> (-12.25%) ⬇️
superset/db_engine_specs/sqlite.py 65.62% <0.00%> (-9.38%) ⬇️
superset/databases/commands/create.py 82.97% <0.00%> (-8.52%) ⬇️
superset/databases/commands/update.py 85.71% <0.00%> (-8.17%) ⬇️
superset/utils/celery.py 82.14% <0.00%> (-3.58%) ⬇️
superset/datasets/api.py 89.38% <0.00%> (-3.36%) ⬇️
superset/models/core.py 86.07% <0.00%> (-3.07%) ⬇️
superset/databases/api.py 87.73% <0.00%> (-2.84%) ⬇️
superset/views/core.py 71.76% <0.00%> (-2.59%) ⬇️
superset/db_engine_specs/postgres.py 97.56% <0.00%> (-2.44%) ⬇️
... and 451 more

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 b2636f0...2087bbc. Read the comment docs.

@rusackas
Copy link
Copy Markdown
Member

another rebase should fix the broken E2E test.

@betodealmeida
Copy link
Copy Markdown
Member Author

Thanks, @rusackas!

Copy link
Copy Markdown
Member

@john-bodley john-bodley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@betodealmeida sorry I don't recall anything beyond the comments in the original PR. Given that there is a similar check elsewhere and this seems to mitigate the problem the change LGTM.

@betodealmeida
Copy link
Copy Markdown
Member Author

Thanks, @john-bodley!

@betodealmeida betodealmeida merged commit 1d9d905 into apache:master Oct 29, 2020
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 First shipped in 1.0.0 labels Mar 12, 2024
qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XS 🚢 1.0.0 First shipped in 1.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants