You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by "ffang (via GitHub)" <gi...@apache.org> on 2025/08/26 00:06:07 UTC
[PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
ffang opened a new pull request, #2571:
URL: https://github.com/apache/cxf/pull/2571
…lient is being shutdown
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #2571:
URL: https://github.com/apache/cxf/pull/2571#issuecomment-3263835178
@ffang apologies, are we OK to merge it or there is something else you are looking into? thanks!
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "eric6iese (via GitHub)" <gi...@apache.org>.
eric6iese commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299742778
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -125,10 +125,18 @@ private static final class RefCount<T extends HttpClient> {
this.policy = policy;
this.clientParameters = clientParameters;
this.finalizer = finalizer;
+ this.count = new AtomicLong(1);
}
RefCount<T> acquire() {
Review Comment:
If am not mistaken, there should be simpler way to implement this. If you change acquire to boolean, then there is no need for an exception or the isClosed-Method.
```java
boolean acquire() {
while (true) {
final long c = count.get();
if (c == 0L) {
return false; // skip closed but not yet removed clients
} else if (count.compareAndSet(c, c + 1)) {
return true;
}
}
}
```
The section below than changes to:
```java
for (final RefCount<HttpClient> p: clients) {
if (cpc.equals(p.policy(), policy) && p.clientParameters().equals(clientParameters) && p.acquire()) {
return p;
}
}
```
To be honest, I am still not sure if this is safe on all paths. If I would write this routine, then I would use a simple long in the refcounter and add a field for the lock above to use for release as well to avoid any possible race condition between these two methods. But that might come at its own costs and complexities.
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,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) {
Review Comment:
While not part of the PR, I wanted to note that Synchronization needs to happen along all read/write-paths of an collection which is not threadsafe, like the clients-ArrayList.
Even a simple operation like clients.size() is not safe to be invoked while another thread manipulates the list. I would recommend to move these lines in the locking-section below.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2333857310
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,14 +207,22 @@ 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 {
Review Comment:
I think this is fine as well, the outer `try` in only to fail fast (we usually only have on client per that matches `cpc.equals(p.policy(), policy) && p.clientParameters().equals(clientParameters))` , thanks @ffang !
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,14 +207,22 @@ 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 {
Review Comment:
I think this is fine as well, the outer `try` was only to fail fast (we usually only have on client per that matches `cpc.equals(p.policy(), policy) && p.clientParameters().equals(clientParameters))` , thanks @ffang !
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2300711115
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,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) {
Review Comment:
I would argue the collection does not need to locked:
- there is no modifications happening
- we also are not particularly worried that `size()` has to be absolutely accurate
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "eric6iese (via GitHub)" <gi...@apache.org>.
eric6iese commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2315329644
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,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) {
Review Comment:
That's correct, if the result does not need to be exact, than this change is not necessary.
Still would not write it that way, but there is no need to change it either. I am a bit pedantic and cautious in this regard since I've been massively burned by multi threading in my youth. ;-)
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299412565
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -127,62 +128,95 @@ private static final class RefCount<T extends HttpClient> {
this.finalizer = finalizer;
}
- RefCount<T> acquire() {
- count.incrementAndGet();
+ /**
+ * Acquire a reference. Throws if shutdown has begun.
+ */
+ synchronized RefCount<T> acquire() {
+ if (shuttingDown) {
Review Comment:
I think we could have a simple fix for (unless I am missing something), do you mind if I push a change ? Thank you.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #2571:
URL: https://github.com/apache/cxf/pull/2571#issuecomment-3222218367
@ffang could you please take a look? thanks again, really appreciate it!
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2300711115
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,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) {
Review Comment:
I would argue the collection does not need to locked:
- there is no modifications happening
- we also are not particularly worried that `size()` has to be absolutely accurate
(In other words, invoking `size()` is absolutely thread-safe, the value may not reflect the true state of the structure accurately)
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "eric6iese (via GitHub)" <gi...@apache.org>.
eric6iese commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2315329644
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -195,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) {
Review Comment:
That's correct, if the result does not need to be exact, than this change is not necessary.
Still would not write it that way, but there is no need to change it eiter. I am a bit pedantic and cautious in this regard since I've been massively burned by multi threading in my youth. ;-)
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299428453
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -127,62 +128,95 @@ private static final class RefCount<T extends HttpClient> {
this.finalizer = finalizer;
}
- RefCount<T> acquire() {
- count.incrementAndGet();
+ /**
+ * Acquire a reference. Throws if shutdown has begun.
+ */
+ synchronized RefCount<T> acquire() {
+ if (shuttingDown) {
Review Comment:
Sure, go ahead please @reta !
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2333534821
##########
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 ,
Sorry, just realized that my previous comments were marked as pending so you can't actually see it.
Please take a look and let me know if my change is OK for you.
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang merged PR #2571:
URL: https://github.com/apache/cxf/pull/2571
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2300713394
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -125,10 +125,18 @@ private static final class RefCount<T extends HttpClient> {
this.policy = policy;
this.clientParameters = clientParameters;
this.finalizer = finalizer;
+ this.count = new AtomicLong(1);
}
RefCount<T> acquire() {
Review Comment:
Thanks, this is also possible implementation option
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2333859992
##########
distribution/src/main/release/samples/pom.xml:
##########
@@ -33,10 +33,10 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<cxf.spring.boot.version>3.4.7</cxf.spring.boot.version>
<cxf.spring.cloud.eureka.version>4.2.2</cxf.spring.cloud.eureka.version>
- <cxf.jetty12.version>12.0.25</cxf.jetty12.version>
- <cxf.netty.version>4.1.124.Final</cxf.netty.version>
+ <cxf.jetty12.version>12.0.26</cxf.jetty12.version>
+ <cxf.netty.version>4.1.126.Final</cxf.netty.version>
Review Comment:
Some rebase / cleanup probably needed
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299411161
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -127,62 +128,95 @@ private static final class RefCount<T extends HttpClient> {
this.finalizer = finalizer;
}
- RefCount<T> acquire() {
- count.incrementAndGet();
+ /**
+ * Acquire a reference. Throws if shutdown has begun.
+ */
+ synchronized RefCount<T> acquire() {
+ if (shuttingDown) {
Review Comment:
```
RefCount<T> acquire() {
while (true) {
final long c = count.get();
if (c == 0L) {
throw new IllegalStateException("The client is already shutdown");
} else if (count.compareAndSet(c, c + 1)) {
break;
}
}
return this;
}
```
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "ffang (via GitHub)" <gi...@apache.org>.
ffang commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299431518
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -127,62 +128,95 @@ private static final class RefCount<T extends HttpClient> {
this.finalizer = finalizer;
}
- RefCount<T> acquire() {
- count.incrementAndGet();
+ /**
+ * Acquire a reference. Throws if shutdown has begun.
+ */
+ synchronized RefCount<T> acquire() {
+ if (shuttingDown) {
Review Comment:
> ```
> RefCount<T> acquire() {
> while (true) {
> final long c = count.get();
> if (c == 0L) {
> throw new IllegalStateException("The client is already shutdown");
> } else if (count.compareAndSet(c, c + 1)) {
> break;
> }
> }
> return this;
> }
> ```
If I read it correctly, with this code we always have IllegalStateException("The client is already shutdown") even the first client tries to acquire it.
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-9162]avoid reusing HttpClientHTTPConduit if the underlying HttpC… [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #2571:
URL: https://github.com/apache/cxf/pull/2571#discussion_r2299432761
##########
rt/transports/http/src/main/java/org/apache/cxf/transport/http/HttpClientHTTPConduit.java:
##########
@@ -127,62 +128,95 @@ private static final class RefCount<T extends HttpClient> {
this.finalizer = finalizer;
}
- RefCount<T> acquire() {
- count.incrementAndGet();
+ /**
+ * Acquire a reference. Throws if shutdown has begun.
+ */
+ synchronized RefCount<T> acquire() {
+ if (shuttingDown) {
Review Comment:
Yes. exactly, we need to change the construction logic a bit since it starts with 0)
--
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: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org