htmx icon indicating copy to clipboard operation
htmx copied to clipboard
verified publisher icon bigskysoftware

Fix: htmx-request class removal for parallel requests

Open tarunbhardwaj opened this issue 1 year ago • 4 comments

Description

This patch addresses a concurrency issue where the same indicator, when used for multiple parallel requests, could result in a negative request count. This issue prevented the proper removal of the htmx-request class upon completion of all requests.

The fix ensures accurate tracking of active requests.

Testing

Test case for multiple requests with same indicator is already covered, but reproducing this race condition in test is hard.

Checklist

  • [x] I have read the contribution guidelines
  • [x] I have targeted this PR against the correct branch (master for website changes, dev for source changes)
  • [ ] This is either a bugfix, a documentation update, or a new feature that has been explicitly approved via an issue
  • [x] I ran the test suite locally (npm run test) and verified that it succeeded

tarunbhardwaj avatar Mar 27 '24 18:03 tarunbhardwaj

is it possible to add a test for this fix?

1cg avatar May 15 '24 18:05 1cg

is it possible to add a test for this fix?

There is already a test, but not for the race condition. Any suggestion on how to reproduce this race condition in test?

tarunbhardwaj avatar Jun 18 '24 08:06 tarunbhardwaj

@tarunbhardwaj In tests, we manually trigger the server answer, which means you should be able to run multiple client actions and only then simulate the server response. Could that work in your case? if so, you'd want to reproduce the issue with the current lib's state and confirm the test reproduces the bug, then ensure your PR fixes it

See this example of the current setup (picked a random test there): https://github.com/bigskysoftware/htmx/blob/c6a89b315b6f964f6d69fa53252c2eb3e63aa8c8/test/core/ajax.js#L15-L18

Telroshan avatar Jun 18 '24 10:06 Telroshan

Is this related to #2860? Although #2860 is merged, I can't see the problem fixed in version 2.0.3.

nikalexis avatar Oct 08 '24 21:10 nikalexis

Hey @nikalexis, If you're still seeing the issue and are interested in contributing, feel free to open a PR with a test case as described above for that race condition I'll be closing this PR as it looks very similar to #2860 that was merged, so it's likely that if the issue persists, it lies elsewhere than what this PR addresses

Telroshan avatar Dec 16 '24 09:12 Telroshan