Skip to content
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

Enable concurrent execution #6357

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

Youssef1313
Copy link
Member

@Youssef1313 Youssef1313 commented Dec 15, 2022

Fixes #2068

@mavasani I'm assuming 2.9.x is the correct branch? Let me know if I should rebase on main branch

Note: I don't see any concurrency issues in the implementation, but this needs to be carefully confirmed.

@Youssef1313 Youssef1313 requested a review from a team as a code owner December 15, 2022 12:01
@mavasani
Copy link
Member

I don't think we want to try updating the 2.9.x branch. It has been dormant for quite a while and unless there is an absolute need we want to avoid fixing anything here.

@Youssef1313 Youssef1313 changed the base branch from 2.9.x to main December 24, 2022 07:52
@codecov
Copy link

codecov bot commented Dec 24, 2022

Codecov Report

Merging #6357 (7a83a9a) into main (5a425e7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6357   +/-   ##
=======================================
  Coverage   96.11%   96.11%           
=======================================
  Files        1361     1361           
  Lines      316034   316035    +1     
  Branches    10192    10192           
=======================================
+ Hits       303741   303746    +5     
+ Misses       9861     9858    -3     
+ Partials     2432     2431    -1     

{
//analysisContext.EnableConcurrentExecution();
context.EnableConcurrentExecution();
Copy link
Member

Choose a reason for hiding this comment

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

So it is safe to enable it now? (The reference in the issue had a comment // TODO: Make analyzer thread-safe. Could not find from history how its removed or who removed it

Copy link
Member Author

Choose a reason for hiding this comment

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

@buyaa-n I'm not entirely sure tbh. As mentioned in PR description:

Note: I don't see any concurrency issues in the implementation, but this needs to be carefully confirmed.

cc @sharwell

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DoNotUseInsecureXSLTScriptExecutionAnalyzer does not support concurrent execution
3 participants