-
-
Notifications
You must be signed in to change notification settings - Fork 8.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
[JENKINS-70922] Remove Prototype Ajax.Request
usage from select.js
#7982
Conversation
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.
This was meant to be a starting point for further exploration, not a final solution. I merely sought to demonstrate that interleaved callbacks from refill operations were interfering with each other and causing a data race.
(1) First, I think we should determine what is firing the second change event. Does it make sense, or is it unnecessary? If we can eliminate that, the entire problem is solved.
(2) If the second change event is necessary, then I think we should explore whether the refill process can be made thread-safe with traditional techniques (e.g., reducing the use of shared data).
Only if the second change event is necessary (1) and it cannot be made thread-safe (2) do I think we should implement mutual exclusion (3) as in my cheesy sleep and retry demonstration.
And if we are to implement mutual exclusion (3), my cheesy sleep and retry demonstration is certainly not a good way to do it. Doing a quick Google search, I found a handful of mutex libraries. Or for a library-less option, anyone can implement a Lamport lock at home (which is essentially a mathematically correct version of my cheesy sleep and retry demonstration).
It's only a single event but there's multiple event handlers registered to the element Verified by removing one of the event handlers, and it only runs once. |
By sticking on a breakpoint and looking at the callstack I tracked the second event listener to this code: Hopefully it can just be safely removed. |
It can't be removed as-is, the steps from jenkinsci/credentials-plugin#23 still reproduce the issue |
That works now, thoughts @basil? (Will need ATH testing as well) |
This is still failing ATH. |
Hmm it's a really weird one. When running through ATH it selects the credential and then it gets reset to none super quickly. It sometimes works too but fails most of the time, and if you pause before selecting it does work |
Tests seem to mostly pass after: 9bee3b1 I'm having to use chrome locally as Firefox isn't working on newer versions which might be why some tests are failing. Let's see how it goes on the pipeline |
That would be evidence that the data race still exists. |
Yes that was resolved by 9bee3b1 |
Aha, this was the real fix, and the change to |
c450379
to
9432504
Compare
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.
ATH is looking good on the latest revision. Nice job taking this across the finish line!
/label ready-for-merge This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback. Thanks! |
See JENKINS-70922.
Testing done
Manually tested by clicking through the UI where it's used adding console.log to see what was using it.
Credentials were fine.
Ran
mvn test -Dtest=hudson.RelativePathTest
which provides test coverage (and it caught an issue with parameters originally)Draft till ran through ATH
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).