fix(sbom): release lock before sleeping in _rate_limit#1896
fix(sbom): release lock before sleeping in _rate_limit#1896mesutoezdil wants to merge 5 commits into
Conversation
PR Review StatusValidation: This is project-valid small, concentrated SBOM tooling work. The goal of avoiding global lock contention across independent package registries is in scope for OpenShell release/SBOM maintenance. Review findings:
Docs: Not needed; this changes internal SBOM tooling behavior and does not alter user-facing CLI/API/TUI/docs behavior. Checks: DCO and vouch are passing. Branch Checks and Helm Lint are still waiting for copy-pr validation, but I am not posting Next state: |
Author Follow-Up NudgeThis PR has been in @mesutoezdil, please respond to the review comments or push an update. If this is no longer planned, please say so and a maintainer can close it out. |
|
Fixed in e067839. Renamed to resolve_licenses_test.py, added test:sbom task to tasks/test.toml. |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Remaining items:
Docs: not needed; this is release/SBOM tooling behavior, not a direct user-facing UX change. Next state: |
time.sleep was called inside the _rate_lock block, blocking all threads from checking their own domain rate limit while one thread slept. With _MAX_WORKERS=12 querying crates.io, npm, and pypi concurrently, this made the thread pool effectively serial. Move the sleep outside the lock so threads for different domains can proceed concurrently.
Use time.monotonic() and store next_req = max(now, last + interval) so concurrent same-domain callers each get a unique future slot rather than all sleeping the same amount. Add regression tests for same-domain spacing and different-domain non-blocking.
Rename test_resolve_licenses.py to resolve_licenses_test.py to match repo *_test.py convention. Add test:sbom task to tasks/test.toml and include it in the top-level test depends so mise run test picks it up.
8eac131 to
e067839
Compare
|
ready for re-check on e067839 |
Re-check After Author UpdateI re-evaluated latest head Disposition: partially resolved. Remaining items:
Docs: not needed; this remains internal SBOM tooling behavior, not a direct user-facing UX change. Checks: I am keeping this in review and not posting Next state: |
…ise lock contention
Re-check After Author UpdateI re-evaluated latest head Disposition: resolved. Remaining items:
Docs: not needed; this remains internal SBOM tooling behavior, not a direct user-facing UX change. Checks: DCO is passing. Branch Checks and Helm Lint are waiting for copy-pr validation, and I have maintainer authority, so I will post Next state: |
|
/ok to test 7c210df |
PR Review StatusValidation: This remains project-valid small, concentrated SBOM tooling work. Review findings:
Docs: not needed; this remains internal SBOM tooling behavior and does not alter user-facing CLI/API/TUI/docs behavior. Checks: Next state: |
Re-check After Author UpdateI re-evaluated latest head Disposition: resolved. Remaining items:
Docs: not needed; this remains internal release/SBOM tooling behavior, not a direct user-facing CLI/API/TUI/docs change. Checks: DCO is passing. Branch Checks and Helm Lint are waiting for copy-pr validation on the new head, and I have maintainer authority, so I will post Next state: |
|
/ok to test 2511ce0 |
Maintainer Approval NeededGator validation and PR monitoring are complete. Validation: This is project-valid small, concentrated SBOM tooling work that removes unnecessary cross-domain lock contention in release/SBOM license resolution. Human maintainer approval or merge decision is now required. |
_rate_limitcalledtime.sleep(wait)inside the_rate_lockblock.While 1 thread slept (0.15s per call), all other threads blocked on the lock, even when querying different domains.
With
_MAX_WORKERS = 12running crates.io, npm, and pypi concurrently, the thread pool was effectively serial.Move
time.sleep(wait)outside the lock.Update
_last_request[domain]before sleeping so subsequent threads see the correct timestamp.Measured concurrent calls to 2 independent domains before and after: