ffang commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2330226820
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -229,23 +207,29 @@ RefCount<HttpClient> computeIfAbsent(final boolean
shareHttpClient, final HTTPCl
// Do not share if it is not allowed for the conduit or cache
capacity is exceeded
if (!shareHttpClient || clients.size() >= MAX_SIZE) {
- return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { }).acquire();
+ return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { });
}
lock.lock();
try {
- for (final RefCount<HttpClient> p: clients) {
- if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
- return p.acquire();
+ try {
+ for (final RefCount<HttpClient> p: clients) {
+ if (p.isClosed() /* skip the closed but not yet
removed clients */) {
+ continue;
+ } else if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
+ return p.acquire();
+ }
}
+ } catch (final IllegalStateException ex) {
+ /* The client is being shutdown */
}
Review Comment:
Hi @reta ,
Just a little suggestion on your commit here, WDYT?
Thanks!
Freeman
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -229,23 +207,29 @@ RefCount<HttpClient> computeIfAbsent(final boolean
shareHttpClient, final HTTPCl
// Do not share if it is not allowed for the conduit or cache
capacity is exceeded
if (!shareHttpClient || clients.size() >= MAX_SIZE) {
- return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { }).acquire();
+ return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { });
}
lock.lock();
try {
- for (final RefCount<HttpClient> p: clients) {
- if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
- return p.acquire();
+ try {
+ for (final RefCount<HttpClient> p: clients) {
+ if (p.isClosed() /* skip the closed but not yet
removed clients */) {
+ continue;
+ } else if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
+ return p.acquire();
+ }
}
+ } catch (final IllegalStateException ex) {
+ /* The client is being shutdown */
}
Review Comment:
Hi @reta ,
I revised accordingly
https://github.com/apache/cxf/pull/2571/commits/f9e374122ed6d014b3d178a425b668a4c4bf009f
If you think this is OK, we can merge the PR.
Thanks
Freeman
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -229,23 +207,29 @@ RefCount<HttpClient> computeIfAbsent(final boolean
shareHttpClient, final HTTPCl
// Do not share if it is not allowed for the conduit or cache
capacity is exceeded
if (!shareHttpClient || clients.size() >= MAX_SIZE) {
- return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { }).acquire();
+ return new RefCount<HttpClient>(supplier.get(), policy,
clientParameters, () -> { });
}
lock.lock();
try {
- for (final RefCount<HttpClient> p: clients) {
- if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
- return p.acquire();
+ try {
+ for (final RefCount<HttpClient> p: clients) {
+ if (p.isClosed() /* skip the closed but not yet
removed clients */) {
+ continue;
+ } else if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
+ return p.acquire();
+ }
}
+ } catch (final IllegalStateException ex) {
+ /* The client is being shutdown */
}
Review Comment:
Hi @reta ,
Nice job!
Just this little concern, should we add try... catch into inner for loop?
Otherwise if any p.acquire() throws, the catch will jump out of the loop early
and proceed to create a new client — which works, but it’s better style and
more robust to handle the exception per iteration so one failing candidate
doesn’t hide other potential matches
Something like
```
- try {
- for (final RefCount<HttpClient> p: clients) {
- if (p.isClosed() /* skip the closed but not yet
removed clients */) {
- continue;
- } else if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
- return p.acquire();
- }
- }
- } catch (final IllegalStateException ex) {
- /* The client is being shutdown */
- }
+ for (final RefCount<HttpClient> p: clients) {
+ try {
+ if (p.isClosed()) { // skip closed but not yet
removed clients
+ continue;
+ }
+ if (cpc.equals(p.policy(), policy) &&
p.clientParameters().equals(clientParameters)) {
+ return p.acquire();
+ }
+ } catch (final IllegalStateException ex) {
+ // candidate raced into shutdown; skip and try next
+ continue;
+ }
+ }
```
Thanks!
Freeman
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
