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

Revert optimization that sometimes caused infinite loops in TreeMaps #793

Merged
merged 4 commits into from
Jun 12, 2021

Conversation

AlexLandau
Copy link
Contributor

@AlexLandau AlexLandau commented Jun 11, 2021

The BuildServicesRegistry is apparently not thread-safe, so we
need to either move this registration to during Gradle configuration
or use some other caching mechanism (e.g. put a cache object on an
extension of the root project).

I have left ConjureRunnerResource intact with the expectation
that it will be either used or removed in a subsequent fix of the
optimization. This PR is just to quickly get to a state where we
don't run into this problem.

==COMMIT_MSG==
Revert a performance optimization for large builds that sometimes resulted in hangs due to infinite loops in TreeMap#get.
==COMMIT_MSG==

The BuildServicesRegistry is apparently not thread-safe, so we
need to either move this registration to during Gradle configuration
or use some other caching mechanism (e.g. put a cache object on an
extension of the root project).

I have left ConjureRunnerResource mostly intact with the expectation
that it will be either used or removed in a subsequent fix of the
optimization.
@changelog-app
Copy link

changelog-app bot commented Jun 11, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Revert a performance optimization for large builds that sometimes reverted in hangs due to infinite loops in TreeMap#get.

Check the box to generate changelog(s)

  • Generate changelog entry

Comment on lines 28 to 29
try {
ConjureRunnerResource.createNewRunner(executable).invoke(project, failedTo, unloggedArgs, loggedArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we would need to close the classloader after execution in this case:

Suggested change
try {
ConjureRunnerResource.createNewRunner(executable).invoke(project, failedTo, unloggedArgs, loggedArgs);
try (ConjureRunner runner = ConjureRunnerResource.createNewRunner(executable)) {
runner.invoke(project, failedTo, unloggedArgs, loggedArgs);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, done

@AlexLandau AlexLandau marked this pull request as ready for review June 11, 2021 22:16
@policy-bot policy-bot bot requested a review from tpetracca June 11, 2021 22:17
@carterkozak
Copy link
Contributor

@AlexLandau Do we have an upstream gradle ticket tracking this? I can't imagine the BuildServicesRegistry would be intentionally non-threadsafe.

Happy to cut a release with this for now given the failures we're seeing internally, that behavior is clearly broken. 👍

@bulldozer-bot bulldozer-bot bot merged commit 2f57499 into develop Jun 12, 2021
@bulldozer-bot bulldozer-bot bot deleted the alandau/revert-optimization-causing-hangs branch June 12, 2021 17:51
@svc-autorelease
Copy link
Collaborator

Released 5.9.0

@AlexLandau
Copy link
Contributor Author

@AlexLandau Do we have an upstream gradle ticket tracking this? I can't imagine the BuildServicesRegistry would be intentionally non-threadsafe.

I filed gradle/gradle#17434. It may be intended that registering a shared service only happens during (single-threaded) configuration, with tasks getting access to the service through a Property; that's how you'd have to use it to take advantage of configuration caching or limiting concurrent usage of the service.

rzpt pushed a commit that referenced this pull request Nov 23, 2021
@rzpt rzpt mentioned this pull request Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants