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

Cleanup threads for CacheBuilderTest and FuturesTest #7320

Closed
wants to merge 1 commit into from

Conversation

aoli-al
Copy link
Contributor

@aoli-al aoli-al commented Jul 19, 2024

This PR addresses #7319. Cleanup threads for CacheBuilderTest and FuturesTest

Copy link

google-cla bot commented Jul 19, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@cpovirk
Copy link
Member

cpovirk commented Jul 19, 2024

(Thanks, I expect to be interested in merging this if you can get the CLA sorted out!)

@aoli-al
Copy link
Contributor Author

aoli-al commented Jul 19, 2024

Thanks! It seems that the CLA is cleared https://github.com/google/guava/pull/7320/checks?check_run_id=27685732789

@cpovirk cpovirk self-assigned this Jul 19, 2024
@cpovirk cpovirk added type=other Miscellaneous activities not covered by other type= labels package=general P2 labels Jul 19, 2024
@ben-manes
Copy link
Contributor

fwiw, technically these should shutdown if the test fails. Typically a try-finally would be ideal, but if you want to avoid that then overriding setUp and tearDown could work by creating a Closer per test instance and registering the shutdown with that.

@aoli-al
Copy link
Contributor Author

aoli-al commented Jul 21, 2024

Will it be okay to make service a class field and then initialize it in each test. tearDown should call service.shutdown if service is not null?

@cpovirk
Copy link
Member

cpovirk commented Jul 21, 2024

I think the most important thing is shutting down when the test succeeds so that there aren't threads hanging around during some later test run that someone tries to attach a debugger to. [edit: Or, of course, it would be bad if we had so many threads that that actually causes resource problems.] And I think we're inconsistent about whether we bother to try to shut down in case of failure.

Let me see whether the internal reviewer is concerned or not, hopefully tomorrow.

copybara-service bot pushed a commit that referenced this pull request Jul 22, 2024
Fixes #7320

RELNOTES=n/a
PiperOrigin-RevId: 654183919
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 package=general type=other Miscellaneous activities not covered by other type= labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants