fix(timepicker): make pyparsing thread safe

[zhaoyongjie]

SUMMARY

make pyparsing thread-safe

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

1. run superset with below

 gunicorn -w 1 --threads 64 -b "0.0.0.0:8088" "superset.app:create_app()"

2. open world bank's data dashboard without error

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

[junlincc]

✅
thanks for the fix!!
With lock

Without lock(consistently getting error by refreshing page)

[villebro]

This seems to fix it consistently for me in local dev env. Let's merge this quickly to unblock master and continue refining/optimizing perf later.

[dpgaspar]

Thank you for the fix, but if pyparser is not thread safe we should start working on removing this dependency

[junlincc]

Thank you for the fix, but if pyparser is not thread safe we should start working on removing this dependency

I will hate to see this new feature go, so will many other users, as we already receive some positive feedback from the community. we should figure out a way to fix it forward instead of simply removing the dependency, right? @dpgaspar

[codecov-io]

Codecov Report

Merging #12489 (fb1a96a) into master (e9d66e9) will decrease coverage by 6.95%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #12489      +/-   ##
==========================================
- Coverage   66.12%   59.16%   -6.96%     
==========================================
  Files        1015      959      -56     
  Lines       49554    46791    -2763     
  Branches     5079     4341     -738     
==========================================
- Hits        32767    27684    -5083     
- Misses      16647    19107    +2460     
+ Partials      140        0     -140     

Flag	Coverage Δ
cypress	50.99% <ø> (+5.49%)	⬆️
javascript	?
python	63.77% <100.00%> (-0.32%)	⬇️

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

Impacted Files	Coverage Δ
superset/utils/date_parser.py	95.13% <100.00%> (+0.02%)	⬆️
...uperset-frontend/src/dashboard/util/dnd-reorder.js	0.00% <0.00%> (-100.00%)	⬇️
...rset-frontend/src/dashboard/util/getEmptyLayout.js	0.00% <0.00%> (-100.00%)	⬇️
...dashboard/components/resizable/ResizableHandle.jsx	0.00% <0.00%> (-100.00%)	⬇️
.../src/dashboard/util/getFilterScopeFromNodesTree.js	0.00% <0.00%> (-93.48%)	⬇️
...set-frontend/src/views/CRUD/alert/ExecutionLog.tsx	11.76% <0.00%> (-88.24%)	⬇️
...src/dashboard/components/gridComponents/Header.jsx	10.52% <0.00%> (-86.85%)	⬇️
superset-frontend/src/components/IconTooltip.tsx	13.33% <0.00%> (-86.67%)	⬇️
...rc/dashboard/components/gridComponents/Divider.jsx	13.33% <0.00%> (-86.67%)	⬇️
...end/src/SqlLab/components/ExploreResultsButton.jsx	8.00% <0.00%> (-84.00%)	⬇️
... and 415 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 e9d66e9...fb1a96a. Read the comment docs.

[dpgaspar]

right @junlincc we should figure out a way to replace the dependency (without removing the feature) not sure if our dependency is light or not. But this needs a more detailed analysis.
I may be wrong but there is the probability that this lock will not be the end of this problem

[ktmud]

I think I may have found another fix:

diff --git a/superset/utils/date_parser.py b/superset/utils/date_parser.py
index aee2c83a0..513c740ab 100644
--- a/superset/utils/date_parser.py
+++ b/superset/utils/date_parser.py
@@ -33,6 +33,7 @@ from pyparsing import (
     Optional as ppOptional,
     ParseException,
     ParseResults,
+    ParserElement,
     pyparsing_common,
     quotedString,
     Suppress,
@@ -40,6 +41,8 @@ from pyparsing import (
 
 from .core import memoized
 
+ParserElement.enablePackrat()
+
 logger = logging.getLogger(__name__)
 
 
@@ -375,7 +378,7 @@ class EvalHolidayFunc:  # pylint: disable=too-few-public-methods
         raise ValueError(_("Unable to find such a holiday: [{}]").format(holiday))
 
 
-@memoized()
+@memoized
 def datetime_parser() -> ParseResults:  # pylint: disable=too-many-locals
     (  # pylint: disable=invalid-name
         DATETIME,

Could you test this diff and see if it fixes things for you?

[ktmud]

Enabling packrat cache seems to have fixed it for me.

I'd assume no other programs are using pyparsing so this should be relatively safe.

[ktmud]

Looks like packrat cache comes with a lock for free: https://github.com/pyparsing/pyparsing/blob/f10fcf8c98ea07a63f8286288ce82f773e7e152e/pyparsing/core.py#L719

So I'm 90% sure this will fix the bug... (with the additional benefit of making things a little faster).

[villebro]

Looks like packrat cache comes with a lock for free: https://github.com/pyparsing/pyparsing/blob/f10fcf8c98ea07a63f8286288ce82f773e7e152e/pyparsing/core.py#L719

So I'm 90% sure this will fix the bug... (with the additional benefit of making things a little faster).

I can confirm that this fixes the problem for me, too 👏 This looks like the perfect solution for this case.

[zhaoyongjie]

Looks like packrat cache comes with a lock for free: https://github.com/pyparsing/pyparsing/blob/f10fcf8c98ea07a63f8286288ce82f773e7e152e/pyparsing/core.py#L719

So I'm 90% sure this will fix the bug... (with the additional benefit of making things a little faster).

I changed this PR to add ParserElement.enablePackrat() and enable cache for datetime_parser, thank you for your finger out this solution

[junlincc]

countless thanks to you all for resolving this.🙏 @ktmud @villebro @zhaoyongjie

[mistercrunch]

This was easily one of the ugliest bug in Superset history: elusive, hard to reproduce, thread-safety-related, ...

Good work tracking it down and finding a viable fix for this one!