You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tinkerpop.apache.org by "spmallette (via GitHub)" <gi...@apache.org> on 2025/06/20 19:20:55 UTC
[PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
spmallette opened a new pull request, #3139:
URL: https://github.com/apache/tinkerpop/pull/3139
https://issues.apache.org/jira/browse/TINKERPOP-3167
There is a chance that the Client.hostConnectionPools can land in a state where it has zero hosts present which leads to the returned ConnectionPool itself to be null. Borrowing a Connection then leads to a NullPointerException.
By preferring a check during init of actual connection pool creation the Client avoids a state where it could grab a null pool. This change doesn't address some of the synchronization issues that likely contributed to allowing the NPE in the first place, but there doesn't seem to be evidence that having an inconsistent state here can lead to issues outside of init().
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159605890
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -543,9 +543,20 @@ protected void initializeImplementation() {
this.initializationFailure = ex;
}
- // throw an error if there is no host available after initializing connection pool.
- if (cluster.availableHosts().isEmpty())
+ // throw an error if there is no host available after initializing connection pool. we used
+ // to test cluster.availableHosts().isEmpty() but checking if we actually have hosts in
+ // the connection pool seems a bit more fireproof. if we look at initializeConnectionSetupForHost
+ // we can see that a successful initialization of the host/connection pool pair is followed by
+ // marking the host available and notifying the load balancing strategy. by relying directly on
+ // the state of hostConnectionPools we ensure that there is actually a concrete
+ // host/connection pool pair. even if the connection pool has immediate problems, it can fallback
+ // to its normal reconnection operation and won't put chooseConnection in a state where it can
+ // get a NPE if hostConnectionPools ends up being empty. it seems as if the safest minimum
+ // requirement for leaving this method is to ensure that at least one ConnectionPool constructor
+ // completed for at least one Host.
+ if (hostConnectionPools.isEmpty()) {
Review Comment:
they never was consistent as best i can tell. the `Host` availability is `Cluster` wide so it's quite possible that you could have had one `Client` with an active `Host` then try to spin up a new one and it fail to connect because the server is down but not mark them unavailable. We'd wait for the original `Client` to mark it - note the test i modified.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159603894
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -543,9 +543,20 @@ protected void initializeImplementation() {
this.initializationFailure = ex;
}
- // throw an error if there is no host available after initializing connection pool.
- if (cluster.availableHosts().isEmpty())
+ // throw an error if there is no host available after initializing connection pool. we used
+ // to test cluster.availableHosts().isEmpty() but checking if we actually have hosts in
+ // the connection pool seems a bit more fireproof. if we look at initializeConnectionSetupForHost
+ // we can see that a successful initialization of the host/connection pool pair is followed by
+ // marking the host available and notifying the load balancing strategy. by relying directly on
+ // the state of hostConnectionPools we ensure that there is actually a concrete
+ // host/connection pool pair. even if the connection pool has immediate problems, it can fallback
+ // to its normal reconnection operation and won't put chooseConnection in a state where it can
+ // get a NPE if hostConnectionPools ends up being empty. it seems as if the safest minimum
+ // requirement for leaving this method is to ensure that at least one ConnectionPool constructor
+ // completed for at least one Host.
+ if (hostConnectionPools.isEmpty()) {
Review Comment:
From what I gather, there is no difference here if there is only 1 client per cluster as the connection is marked available in the cluster at the same time that the connection pool is added to `hostConnectionPools` for the client: https://github.com/apache/tinkerpop/blob/3a48401d83656a31c459f541550b62be67084970/gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java#L589C1-L592C38
If this change is fixing some NPE error, to me that's implying that something in the ConnectionPool constructor is failing to mark a host as unavailable if the initialization was unsuccessful.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159580880
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -543,9 +543,20 @@ protected void initializeImplementation() {
this.initializationFailure = ex;
}
- // throw an error if there is no host available after initializing connection pool.
- if (cluster.availableHosts().isEmpty())
+ // throw an error if there is no host available after initializing connection pool. we used
+ // to test cluster.availableHosts().isEmpty() but checking if we actually have hosts in
+ // the connection pool seems a bit more fireproof. if we look at initializeConnectionSetupForHost
+ // we can see that a successful initialization of the host/connection pool pair is followed by
+ // marking the host available and notifying the load balancing strategy. by relying directly on
+ // the state of hostConnectionPools we ensure that there is actually a concrete
+ // host/connection pool pair. even if the connection pool has immediate problems, it can fallback
+ // to its normal reconnection operation and won't put chooseConnection in a state where it can
+ // get a NPE if hostConnectionPools ends up being empty. it seems as if the safest minimum
+ // requirement for leaving this method is to ensure that at least one ConnectionPool constructor
+ // completed for at least one Host.
+ if (hostConnectionPools.isEmpty()) {
Review Comment:
This change seems OK but I am wondering if there is a deeper issue in that `hostConnectionPools.isEmpty()` has somehow become inconsistent with `cluster.availableHosts().isEmpty()`?
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on code in PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#discussion_r2159628397
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Client.java:
##########
@@ -543,9 +543,20 @@ protected void initializeImplementation() {
this.initializationFailure = ex;
}
- // throw an error if there is no host available after initializing connection pool.
- if (cluster.availableHosts().isEmpty())
+ // throw an error if there is no host available after initializing connection pool. we used
+ // to test cluster.availableHosts().isEmpty() but checking if we actually have hosts in
+ // the connection pool seems a bit more fireproof. if we look at initializeConnectionSetupForHost
+ // we can see that a successful initialization of the host/connection pool pair is followed by
+ // marking the host available and notifying the load balancing strategy. by relying directly on
+ // the state of hostConnectionPools we ensure that there is actually a concrete
+ // host/connection pool pair. even if the connection pool has immediate problems, it can fallback
+ // to its normal reconnection operation and won't put chooseConnection in a state where it can
+ // get a NPE if hostConnectionPools ends up being empty. it seems as if the safest minimum
+ // requirement for leaving this method is to ensure that at least one ConnectionPool constructor
+ // completed for at least one Host.
+ if (hostConnectionPools.isEmpty()) {
Review Comment:
The mechanics of that sound a bit strange to me, but I recall that has all been subject to considerable deliberation and testing in the past so I won't weigh in on it here. I agree that given those mechanics, it makes the most sense to use the client's `hostConnectionPools` to verify availability to avoid any influence from other clients on the same cluster.
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "xiazcy (via GitHub)" <gi...@apache.org>.
xiazcy commented on PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#issuecomment-2992650061
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "Cole-Greer (via GitHub)" <gi...@apache.org>.
Cole-Greer commented on PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#issuecomment-2992711038
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "andreachild (via GitHub)" <gi...@apache.org>.
andreachild commented on PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139#issuecomment-2992656030
VOTE +1
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TINKERPOP-3167 Fixed NPE when borrowing a connection [tinkerpop]
Posted by "spmallette (via GitHub)" <gi...@apache.org>.
spmallette merged PR #3139:
URL: https://github.com/apache/tinkerpop/pull/3139
--
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: commits-unsubscribe@tinkerpop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org