-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: timeout context manager on Windows #13041
fix: timeout context manager on Windows #13041
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13041 +/- ##
===========================================
+ Coverage 53.06% 66.99% +13.93%
===========================================
Files 489 489
Lines 17314 28794 +11480
Branches 4482 0 -4482
===========================================
+ Hits 9187 19291 +10104
- Misses 8127 9503 +1376
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
LGTM - this has probably been the main barrier causing Superset no not be supported on Windows. Cautiously optimistic that we could start supporting this platform, too.
self, type: Any, value: Any, traceback: TracebackType | ||
) -> None: | ||
self.timer.cancel() | ||
if type is KeyboardInterrupt: # raised by _thread.interrupt_main |
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.
Surprised that this is a KeyboardInterrupt
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.
Me too!
927888f
to
87b1bb1
Compare
87b1bb1
to
0b978ff
Compare
* fix: timeout decorator in Windows * Fix lint
SUMMARY
We currently use a context manager to timeout queries that run in SQL Lab, eg:
The context manager uses
SIGALRM
, which is not available in Windows, resulting in an error (see #12824).Even though Windows is not technically supported by Superset, this is a simple fix. I introduced a new timeout context manager based on a timer. This PR changes Superset to use that context manager only when running on Windows. In theory we could replace the old one, but I thought it would be safer to introduce the new one in parallel first.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
I installed Python under a Windows VPS and tested the context manager with this script:
(I didn't use
sleep()
because the_thread.interrupt_main
doesn't interrupt it.)Then running it causes the main thread to be aborted after 1 second:
ADDITIONAL INFORMATION