-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
Fix post link race condition #26124
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
base: master
Are you sure you want to change the base?
Fix post link race condition #26124
Conversation
| headers: crumb.wrap({}), | ||
| }).then((rsp) => { | ||
| if (rsp.ok) { | ||
| if (rsp.status === 201) { |
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.
please ensure you create fresh branches from latest master... this is unrelated.
(and don't respond to this with an AI comment)
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.
Thanks for the feedback. I'll update the comment and simplify the href logic to read directly from the data attribute on line 16. Will also make sure to create fresh branches from latest master
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.
Sorry for the AI generated replies . I should have been more thoughtful. Thanks for the feedback, I will make sure future responses are genuine.
| } | ||
|
|
||
| // Restore href from data attribute if it was stored (prevents race condition) | ||
| if (el.dataset.taskHref) { |
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.
I'm not sure that this should be done till the onclick is registered, you can always just determine what href is needed on L16 so it fetches the right url
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.
Done , updated to read directly from the data attribute on line 12 instead of storing/restoring.
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.
you haven't pushed
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.
Thanks for the reminder. I have pushed the changes now.
Applied your suggestion to use null instead of an empty string for data-task-href — that's in the latest commit. Also updated the href logic to read directly from the data attribute as you suggested earlier.
Regarding the configurable.js deletion: that came in from a merge and wasn't part of the original PR. I had removed all references to it, and the code works with the standard task.js POST handling
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.
Please restore this file.
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.
I see you asked to restore configurable.js. The deletion came in via a merge and wasn't part of the original race condition fix. I've removed all references to it, and the code works without it using the standard task.js POST handling. Should I restore the file, or is the current approach acceptable?
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.
Its still there in master: https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/lib/hudson/project/configurable/configurable.js, if we can verify its not needed then we can remove it
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.
The file was already in master. I removed it in this PR since I switched to using the standard task.js handling. It is not needed for my changes, but we should verify it is not used elsewhere before removing it from master
| data-task-success="${%Done.}" | ||
| data-task-failure="${%Failed.}" | ||
| data-task-post="${attrs.post}" | ||
| data-task-href="${attrs.post == 'true' ? href : ''}" |
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.
I think if you use null the tag would be omitted
| data-task-href="${attrs.post == 'true' ? href : ''}" | |
| data-task-href="${attrs.post == 'true' ? href : null}" |
|
Please make sure to test all scenarios. What is also needed is to test a job with parameters and then click the |
|
Non-parameterized job: "Build Now" works when clicked right after page load and after it fully loads. No POST errors, builds schedule correctly, notifications show. Parameterized job: "Build with Parameters" opens the dialog, parameters submit, builds schedule with the parameters. Both work as expected. The fix handles the race condition and doesn't break parameterized jobs. |
|
Please see #7635 why that script is there. Technically it is not needed, but serves as an example how to implement the callback feature. |
7c8216e to
9106f80
Compare
| <j:if test="${it.buildable}"> | ||
| <st:adjunct includes="lib.hudson.project.configurable.configurable"/> | ||
| <l:task href="${url}/build?delay=0sec" icon="icon-clock icon-md" permission="${it.BUILD}" post="${!it.parameterized}" data-callback="lib_hudson_project_configurable_build_now_callback" data-build-failure="${%buildFailed}" data-build-success="${%Build scheduled}" data-parameterized="${it.parameterized}" title="${it.buildNowText}"/> | ||
| <l:task href="${url}/build?delay=0sec" icon="icon-clock icon-md" permission="${it.BUILD}" post="${!it.parameterized}" data-task-failure="${%buildFailed}" data-task-success="${%Build scheduled}" title="${it.buildNowText}"/> |
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.
Need to restore the data-callback and the data-parameterized and include the adjunct with the configurable
|
The ATH failures are due to the fix. The tests click Build Now immediately. My change sets href="#" until the JS loads to prevent the race condition for users. This causes the tests to click nothing and timeout because they don't wait for the JS. Should I revert to the real URL risking the race condition for users to pass these tests or is there a way to make the ATH tests wait for the JS handler? |
Change ATH to wait |
36096a3 to
e207c18
Compare
|
@timja @mawinter69 |
So if I understand the above correctly this does not fix the issue then? I as a user was seeing "this requires a POST" before when clicking on various tasks manually during regular usage (and it was not a rare occurence), whereas now I will click and nothing happens at all? is there a reason we are using |
@jtnord You're right, the current approach isn't great UX - clicking and having nothing happen is arguably worse than seeing the POST error. Why onload :- When the code was written DOMContentLoaded had spotty browser support. The onload waits for all assets which creates this race window where the link is visible but handlers are not attached yet. Switching to DOMContentLoaded would fix this at the root - handlers attach right when DOM is ready instead of waiting for assets The concern is whether any existing behaviors actually depend on assets being loaded when they run. I am not sure if that is the case anywhere. Alternatively we could keep href="#" but show a "Please wait..." message when clicked too early so the user at least gets feedback. Which approach would you prefer? |
I may be missing something but how can you show a message that requires javascript, whilst also adhering to CSP that says javascript must be in external files? Doesn't this come down to the same issue that link is not updated to POST? (the external javascript file has not yet been loaded)? |
You are right - showing a message also requires JS, so same problem. The fix should be switching Behaviour.js from onload to DOMContentLoaded. This attaches handlers when DOM is ready (before assets) eliminating the race window. Can I revert the href="#" change and test this approach instead. |
How about giving the |
|
I will try the DOMContentLoaded approach and revert the href="#" change to see how it behaves in practice. |
5fae470 to
f05a46c
Compare
| * @since TODO | ||
| */ | ||
| @Restricted(NoExternalUse.class) | ||
| public static boolean getUseDOMContentLoaded() { |
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.
I don't think this should be in place unless an issue is actually found, it just needs testing properly
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.
I will remove the toggle and make the switch to DOMContentLoaded unconditional. I will perform more testing across all the pages and update the PR shortly
f05a46c to
f8f477c
Compare
|
I have removed the toggle and made the switch unconditional. I tested this across job configs, node management, and the plugin manager specifically verified that dynamic form elements , help tooltips, cron validation, and AJAX filtering in the plugin manager all work correctly without the toggle |
| Behaviour.addLoadEvent(function () { | ||
| Behaviour.apply(); | ||
| }); | ||
| if (document.addEventListener) { |
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.
you do not need this check
see #26087 for the known failures. I saw failures and did no investigate on them as that PR ultimately did not fix the issue I was attempting to fix. |
|
After reviewing the PR for the POST link fix and the UX discussion, here’s my proposed ideal solution: Solution:
Why:
In short: DOMContentLoaded + real URL = consistent, safe user experience without errors or clicks that don’t do anything. What do you think? |
|
I agree with your approach. I have already made the DOMContentLoaded change in behavior.js and also removed the legacy browser check that @timja mentioned. Tested it across job configs, node management, plugin manager and everything works fine including the dynamic forms, tooltips, and cron validation. For the href revert part, should I include that in this pull request or keep it separate? |
|
@timja any updates on this . Do this pr need any further change or testing ? |
|
stop pinging me. I will review as I have time, anyone else can review it |
|
Sorry for the inconvenience |
Fixes #16832
Description
Fixes a race condition that caused sporadic "This URL requires POST" errors when clicking links that require POST requests.
Root Cause:
Links requiring POST are rendered with
hrefattributes set immediately. JavaScript handlers are attached asynchronously viaBehaviour.specify()which runs onwindow.onload. If a user clicks beforewindow.onloadcompletes, the browser follows thehrefas a GET request, causing "This URL requires POST" errors.Solution:
data-task-hreffor POST linkshref="#"initially to prevent navigationBehaviour.specify()runs, restore the realhrefand attach the handlerThis follows the same pattern used in
link.js(lines 2-5) which already handles POST links correctly.Testing done
Manual testing performed:
window.onloadcompletes)Test scenarios:
Browser testing:
Note: This is a frontend JavaScript change that prevents a race condition. The fix ensures handlers are always attached before links become navigable, eliminating the timing window where the race condition could occur.
Screenshots (UI changes only)
N/A - No visible UI changes. This is a behind-the-scenes fix that prevents errors.
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).evalcalls. All JavaScript is in separate.jsfiles.Desired reviewers
@timja @mawinter69