You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by GitBox <gi...@apache.org> on 2021/06/18 20:54:02 UTC
[GitHub] [httpcomponents-core] rschmitt opened a new pull request #293: Fix data race in StrictConnPool
rschmitt opened a new pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] rschmitt commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
rschmitt commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r654649739
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
|Thread A | Thread B|
|-|-|
|Set `completed` to true ||
||Call `isDone()`, which returns `true`|
||Call `getResult()`/`getException()`, which return `null`|
|Set `ex`/`result`||
I found this race condition by inspection. I'm trying to track down a connection leak that we're continuing to get reports about. In the absence of a repro, I've resorted to auditing the code. I don't know if this race condition results in an actual bug, or is just benign, but either way there's no reason not to fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r654357416
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
@rschmitt Could you please give some details about the race?
If we abolutely have to synchronize here the use of AtomicBoolean for that end feels wrong.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] carterkozak commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r655591889
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
I've created a PR with the proposed change here: #294
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] rschmitt closed pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
rschmitt closed pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] carterkozak commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r655591889
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
I've created a PR with the proposed change here: #294
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] ok2c commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
ok2c commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r654782487
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
@carterkozak Makes sense to me.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] carterkozak commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r654678265
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
I suppose an alternative and potentially simpler solution would be:
AtomicBoolean completed is used exclusively to guard internally _setting_ `this.result` and `this.completed`, but is no longer used by `public boolean isDone()`
isDone may be updated to make two volatile reads.
```java
public boolean isDone() {
return result != null || ex != null;
}
```
This way isDone and getException/getResult always agree. What do you think?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] rschmitt closed pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
rschmitt closed pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
[GitHub] [httpcomponents-core] carterkozak commented on a change in pull request #293: Fix data race in StrictConnPool
Posted by GitBox <gi...@apache.org>.
carterkozak commented on a change in pull request #293:
URL: https://github.com/apache/httpcomponents-core/pull/293#discussion_r654106072
##########
File path: httpcore5/src/main/java/org/apache/hc/core5/pool/StrictConnPool.java
##########
@@ -699,14 +699,18 @@ public boolean isDone() {
}
public void failed(final Exception ex) {
- if (this.completed.compareAndSet(false, true)) {
- this.ex = ex;
+ synchronized (this.completed) {
Review comment:
Perhaps we replace the atomic boolean with an atomic reference to the result/exception, and avoid synchronization?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org