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

fix: do not read response from a closed client #438

Merged
merged 2 commits into from
Feb 17, 2025

Conversation

saranyailla
Copy link
Member

Issue #, if available:

Description of changes:

Why is this change necessary:
Whenever a stream response of the associated client devices is read, it uses the underlying GGv2 client. But with try-with-resources, the client is closed as soon as the response is returned and gives the following error

Exception in thread "main" java.lang.IllegalStateException: Connection pool shut down
	at org.apache.http.util.Asserts.check(Asserts.java:34)
	at org.apache.http.impl.conn.PoolingHttpClientConnectionManager.requestConnection(PoolingHttpClientConnectionManager.java:269)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:567)
	at software.amazon.awssdk.http.apache.internal.conn.ClientConnectionManagerFactory$Handler.invoke(ClientConnectionManagerFactory.java:80)
	at com.sun.proxy.$Proxy0.requestConnection(Unknown Source)

With this change, we read the full response from the stream and convert it to a list so we don't depend on the client after the response is returned.

How was this change tested:

Any additional information or context required to review the change:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@saranyailla saranyailla force-pushed the fix-background-refresh branch from 9492fe9 to ebfaf77 Compare February 14, 2025 21:44
Copy link

Code Coverage Report

File Coverage
All files 74%

Minimum allowed coverage is 50%

Generated by 🐒 cobertura-action against 22c0bc6

Copy link

Benchmark Results

Benchmark Score
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_thing_name_variable_WHEN_auth_request_THEN_successful_auth 1258384.36 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_policy_with_wildcards_WHEN_auth_request_THEN_successful_auth 222568.12 ops/s
com.aws.greengrass.clientdevices.auth.benchmark.AuthorizationBenchmarks.GIVEN_single_group_permission_WHEN_simple_auth_request_THEN_successful_auth 2491344.45 ops/s

@junfuchen99 junfuchen99 merged commit 1dbaf82 into main Feb 17, 2025
4 checks passed
@junfuchen99 junfuchen99 deleted the fix-background-refresh branch February 17, 2025 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants