dsmiley commented on code in PR #2259:
URL: https://github.com/apache/solr/pull/2259#discussion_r1520495658
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -278,64 +285,74 @@ private HttpResponse<InputStream> doPutOrPost(
return response;
}
- private volatile boolean headRequested;
- private volatile boolean headSucceeded;
-
/**
- * TODO: we should only try this if not set to Http/1.1 and not TLS
- *
- *
- * This is a workaround for the case where the first request using
- * this client:
+ * This is a workaround for the case where:
*
- * (1) no SSL/TLS (2) using POST with stream and (3) using Http/2
+ * <p>(1) no SSL/TLS (2) using POST with stream and (3) using Http/2
*
- * The JDK Http Client will send an upgrade request over Http/1
- * along with request content in the same request. However,
- * the Jetty Server underpinning Solr does not accept this.
+ * <p>The JDK Http Client will send an upgrade request over Http/1 along
with request content in
+ * the same request. However, the Jetty Server underpinning Solr does not
accept this.
*
- * By sending a ping before any real requests occur, the client
- * knows if Solr can accept Http/2, and no additional
- * upgrade requests will be sent.
+ * <p>We send a HEAD request first, then the client knows if Solr can accept
Http/2, and no
+ * additional upgrade requests will be sent.
*
- * See https://bugs.openjdk.org/browse/JDK-8287589
- * See
https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
+ * <p>See https://bugs.openjdk.org/browse/JDK-8287589 See
+ * https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
*
- * We only try once, and if it fails, we downgrade to Http/1
+ * <p>We only try once, and if it fails, we downgrade to Http/1
*
* @param url the url with no request parameters
* @return true if success
*/
- private synchronized boolean maybeTryHeadRequest(String url) {
- if(headRequested) {
+ private boolean maybeTryHeadRequest(String url) {
+ if (forceHttp11 || url == null ||
url.toLowerCase(Locale.ROOT).startsWith("https://";)) {
+ return true;
+ }
+ return maybeTryHeadRequestSync(url);
+ }
+
+ protected volatile boolean headRequested; // must be threadsafe
+ private boolean headSucceeded; // must be threadsafe
+
+ private synchronized boolean maybeTryHeadRequestSync(String url) {
+ if (headRequested) {
return headSucceeded;
}
+
URI uriNoQueryParams;
try {
uriNoQueryParams = new URI(url);
- } catch(URISyntaxException e) {
- headSucceeded = false;
+ } catch (URISyntaxException e) {
+
+ // If the url is invalid, let a subsequent request try again.
return false;
}
- HttpRequest.Builder headReqB =
HttpRequest.newBuilder(uriNoQueryParams).method("HEAD",
HttpRequest.BodyPublishers.noBody());
+ HttpRequest.Builder headReqB =
+ HttpRequest.newBuilder(uriNoQueryParams)
+ .method("HEAD", HttpRequest.BodyPublishers.noBody());
decorateRequest(headReqB, new QueryRequest());
try {
httpClient.send(headReqB.build(),
HttpResponse.BodyHandlers.discarding());
headSucceeded = true;
- } catch(IOException ioe) {
+ } catch (IOException ioe) {
log.warn("Could not issue HEAD request to {} ", url, ioe);
headSucceeded = false;
- } catch(InterruptedException ie) {
+ } catch (InterruptedException ie) {
Thread.currentThread().interrupt();
headSucceeded = false;
} finally {
+
+ // The HEAD request is tried only once. All future requests will skip
this check.
headRequested = true;
+
+ if (!headSucceeded) {
+ log.info("All insecure POST requests with a chunked body will use
http/1.1");
Review Comment:
Would replacing "insecure" with "HTTP" or "unencrypted" or "non-SSL" be
fine? To me, there's something about the word "insecure" that refers to
overall security posture, thus implying the user is negligent here which is not
necessarily the case.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -278,64 +285,74 @@ private HttpResponse<InputStream> doPutOrPost(
return response;
}
- private volatile boolean headRequested;
- private volatile boolean headSucceeded;
-
/**
- * TODO: we should only try this if not set to Http/1.1 and not TLS
- *
- *
- * This is a workaround for the case where the first request using
- * this client:
+ * This is a workaround for the case where:
*
- * (1) no SSL/TLS (2) using POST with stream and (3) using Http/2
+ * <p>(1) no SSL/TLS (2) using POST with stream and (3) using Http/2
*
- * The JDK Http Client will send an upgrade request over Http/1
- * along with request content in the same request. However,
- * the Jetty Server underpinning Solr does not accept this.
+ * <p>The JDK Http Client will send an upgrade request over Http/1 along
with request content in
+ * the same request. However, the Jetty Server underpinning Solr does not
accept this.
*
- * By sending a ping before any real requests occur, the client
- * knows if Solr can accept Http/2, and no additional
- * upgrade requests will be sent.
+ * <p>We send a HEAD request first, then the client knows if Solr can accept
Http/2, and no
+ * additional upgrade requests will be sent.
*
- * See https://bugs.openjdk.org/browse/JDK-8287589
- * See
https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
+ * <p>See https://bugs.openjdk.org/browse/JDK-8287589 See
+ * https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
*
- * We only try once, and if it fails, we downgrade to Http/1
+ * <p>We only try once, and if it fails, we downgrade to Http/1
*
* @param url the url with no request parameters
* @return true if success
*/
- private synchronized boolean maybeTryHeadRequest(String url) {
- if(headRequested) {
+ private boolean maybeTryHeadRequest(String url) {
Review Comment:
Would protected scope be useful to allow a user to change?
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -64,6 +62,12 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * A SolrClient implementation that communicates to a Solr server using the
built-in Java 11+ Http
+ * Client. This client is targeted for those users who wish to minimize
application dependencies.
+ * This client will connect to solr using Http/2 but can seamlessly downgrade
to Http/1.1 when
+ * connecting to Solr hosts running on older versions.
+ */
public class HttpSolrJdkClient extends HttpSolrClientBase {
Review Comment:
If I suggested the "Jdk" between Solr & Client then I misspoke; I recall
HttpJdkSolrClient or "JdkHttpSolrClient" (I like the second) because
"SolrClient" is the important base type here.
##########
gradle/testing/randomization/policies/solr-tests.policy:
##########
Review Comment:
This is only for tests; must we be so specific on the URLs?
I'd like to remind us why we even use the security manager for tests in the
first place. It's for the file system to tame tests. Makes sense; a clever
solution. I never recall anyone advocating that more than that is important.
Yet it was a slippery slope of "hey why not" for all these other things. But
it's plain here that this is burdensome for us to maintain; not to mention
Oracle tells us it's all going away anyhow. I'll raise this on the dev list.
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java:
##########
@@ -278,64 +285,74 @@ private HttpResponse<InputStream> doPutOrPost(
return response;
}
- private volatile boolean headRequested;
- private volatile boolean headSucceeded;
-
/**
- * TODO: we should only try this if not set to Http/1.1 and not TLS
- *
- *
- * This is a workaround for the case where the first request using
- * this client:
+ * This is a workaround for the case where:
*
- * (1) no SSL/TLS (2) using POST with stream and (3) using Http/2
+ * <p>(1) no SSL/TLS (2) using POST with stream and (3) using Http/2
*
- * The JDK Http Client will send an upgrade request over Http/1
- * along with request content in the same request. However,
- * the Jetty Server underpinning Solr does not accept this.
+ * <p>The JDK Http Client will send an upgrade request over Http/1 along
with request content in
+ * the same request. However, the Jetty Server underpinning Solr does not
accept this.
*
- * By sending a ping before any real requests occur, the client
- * knows if Solr can accept Http/2, and no additional
- * upgrade requests will be sent.
+ * <p>We send a HEAD request first, then the client knows if Solr can accept
Http/2, and no
+ * additional upgrade requests will be sent.
*
- * See https://bugs.openjdk.org/browse/JDK-8287589
- * See
https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
+ * <p>See https://bugs.openjdk.org/browse/JDK-8287589 See
+ * https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870
*
- * We only try once, and if it fails, we downgrade to Http/1
+ * <p>We only try once, and if it fails, we downgrade to Http/1
*
* @param url the url with no request parameters
* @return true if success
*/
- private synchronized boolean maybeTryHeadRequest(String url) {
- if(headRequested) {
+ private boolean maybeTryHeadRequest(String url) {
+ if (forceHttp11 || url == null ||
url.toLowerCase(Locale.ROOT).startsWith("https://";)) {
+ return true;
+ }
+ return maybeTryHeadRequestSync(url);
+ }
+
+ protected volatile boolean headRequested; // must be threadsafe
+ private boolean headSucceeded; // must be threadsafe
+
+ private synchronized boolean maybeTryHeadRequestSync(String url) {
+ if (headRequested) {
return headSucceeded;
}
+
URI uriNoQueryParams;
try {
uriNoQueryParams = new URI(url);
- } catch(URISyntaxException e) {
- headSucceeded = false;
+ } catch (URISyntaxException e) {
+
Review Comment:
remove blank
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
