dsmiley commented on code in PR #2259: URL: https://github.com/apache/solr/pull/2259#discussion_r1506455155
########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java: ########## @@ -0,0 +1,453 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.client.solrj.impl; + +import java.io.IOException; +import java.io.InputStream; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; +import java.lang.invoke.MethodHandles; +import java.net.CookieHandler; +import java.net.InetSocketAddress; +import java.net.ProxySelector; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.net.http.HttpTimeoutException; +import java.time.Duration; +import java.time.temporal.ChronoUnit; +import java.util.Collection; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import javax.net.ssl.SSLContext; +import org.apache.solr.client.solrj.ResponseParser; +import org.apache.solr.client.solrj.SolrRequest; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.request.RequestWriter; +import org.apache.solr.client.solrj.util.ClientUtils; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.util.ContentStream; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.ObjectReleaseTracker; +import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.eclipse.jetty.util.BlockingArrayQueue; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class HttpSolrJdkClient extends HttpSolrClientBase { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private static final String USER_AGENT = + "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] 1.0"; + + protected HttpClient client; + + protected ExecutorService executor; + + private boolean shutdownExecutor; + + protected HttpSolrJdkClient(String serverBaseUrl, HttpSolrJdkClient.Builder builder) { + super(serverBaseUrl, builder); + + HttpClient.Redirect followRedirects = + Boolean.TRUE.equals(builder.followRedirects) + ? HttpClient.Redirect.NORMAL + : HttpClient.Redirect.NEVER; + HttpClient.Builder b = HttpClient.newBuilder().followRedirects(followRedirects); + if (builder.sslContext != null) { + b.sslContext(builder.sslContext); + } + + if (builder.executor != null) { + this.executor = builder.executor; + this.shutdownExecutor = false; + } else { + BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(32, 32); + this.executor = + new ExecutorUtil.MDCAwareThreadPoolExecutor( + 4, + 256, + 60, + TimeUnit.SECONDS, + queue, + new SolrNamedThreadFactory(this.getClass().getSimpleName())); + this.shutdownExecutor = true; + } + b.executor(this.executor); + + if (builder.useHttp1_1) { + b.version(HttpClient.Version.HTTP_1_1); Review Comment: If you expressly set the version to HTTP 2, can we avoid the ping altogether? ########## solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpSolrJdkClient.java: ########## @@ -0,0 +1,453 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.solr.client.solrj.impl; + +import java.io.IOException; +import java.io.InputStream; +import java.io.PipedInputStream; +import java.io.PipedOutputStream; +import java.lang.invoke.MethodHandles; +import java.net.CookieHandler; +import java.net.InetSocketAddress; +import java.net.ProxySelector; +import java.net.URI; +import java.net.URISyntaxException; +import java.net.http.HttpClient; +import java.net.http.HttpRequest; +import java.net.http.HttpResponse; +import java.net.http.HttpTimeoutException; +import java.time.Duration; +import java.time.temporal.ChronoUnit; +import java.util.Collection; +import java.util.Locale; +import java.util.Map; +import java.util.Objects; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.TimeUnit; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import javax.net.ssl.SSLContext; +import org.apache.solr.client.solrj.ResponseParser; +import org.apache.solr.client.solrj.SolrRequest; +import org.apache.solr.client.solrj.SolrServerException; +import org.apache.solr.client.solrj.request.RequestWriter; +import org.apache.solr.client.solrj.util.ClientUtils; +import org.apache.solr.common.SolrException; +import org.apache.solr.common.params.ModifiableSolrParams; +import org.apache.solr.common.util.ContentStream; +import org.apache.solr.common.util.ExecutorUtil; +import org.apache.solr.common.util.NamedList; +import org.apache.solr.common.util.ObjectReleaseTracker; +import org.apache.solr.common.util.SolrNamedThreadFactory; +import org.eclipse.jetty.util.BlockingArrayQueue; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +public class HttpSolrJdkClient extends HttpSolrClientBase { + private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass()); + + private static final String USER_AGENT = + "Solr[" + MethodHandles.lookup().lookupClass().getName() + "] 1.0"; + + protected HttpClient client; + + protected ExecutorService executor; + + private boolean shutdownExecutor; + + protected HttpSolrJdkClient(String serverBaseUrl, HttpSolrJdkClient.Builder builder) { + super(serverBaseUrl, builder); + + HttpClient.Redirect followRedirects = + Boolean.TRUE.equals(builder.followRedirects) + ? HttpClient.Redirect.NORMAL + : HttpClient.Redirect.NEVER; + HttpClient.Builder b = HttpClient.newBuilder().followRedirects(followRedirects); + if (builder.sslContext != null) { + b.sslContext(builder.sslContext); + } + + if (builder.executor != null) { + this.executor = builder.executor; + this.shutdownExecutor = false; + } else { + BlockingArrayQueue<Runnable> queue = new BlockingArrayQueue<>(32, 32); + this.executor = + new ExecutorUtil.MDCAwareThreadPoolExecutor( + 4, + 256, + 60, + TimeUnit.SECONDS, + queue, + new SolrNamedThreadFactory(this.getClass().getSimpleName())); + this.shutdownExecutor = true; + } + b.executor(this.executor); + + if (builder.useHttp1_1) { + b.version(HttpClient.Version.HTTP_1_1); + } + + if (builder.cookieHandler != null) { + b.cookieHandler(builder.cookieHandler); + } + + if (builder.proxyHost != null) { + if (builder.proxyIsSocks4) { + log.warn( + "Socks4 is likely not supported by this client. See https://bugs.openjdk.org/browse/JDK-8214516";); + } + b.proxy(ProxySelector.of(new InetSocketAddress(builder.proxyHost, builder.proxyPort))); + } + this.client = b.build(); + updateDefaultMimeTypeForParser(); + + // This is a workaround for the case where the first request using + // this client: + // + // (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. + // + // 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. + // + // See https://bugs.openjdk.org/browse/JDK-8287589 + // See https://github.com/jetty/jetty.project/issues/9998#issuecomment-1614216870 + // + try { + ping(); + } catch (Exception e) { + // ignore + } + + assert ObjectReleaseTracker.track(this); + } + + @Override + public NamedList<Object> request(SolrRequest<?> solrRequest, String collection) + throws SolrServerException, IOException { + checkClosed(); + if (ClientUtils.shouldApplyDefaultCollection(collection, solrRequest)) { + collection = defaultCollection; + } + String url = getRequestPath(solrRequest, collection); + ResponseParser parser = responseParser(solrRequest); + ModifiableSolrParams queryParams = initalizeSolrParams(solrRequest); + HttpResponse<InputStream> resp = null; + try { + var reqb = HttpRequest.newBuilder(); + switch (solrRequest.getMethod()) { + case GET: + { + resp = doGet(url, reqb, solrRequest, queryParams); + break; + } + case POST: + { + resp = doPutOrPost(url, false, reqb, solrRequest, queryParams); + break; + } + case PUT: + { + resp = doPutOrPost(url, true, reqb, solrRequest, queryParams); + break; + } + default: + { + throw new IllegalStateException("Unsupported method: " + solrRequest.getMethod()); + } + } + return processErrorsAndResponse(solrRequest, parser, resp, url); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException(e); + } catch (HttpTimeoutException e) { + throw new SolrServerException( + "Timeout occurred while waiting response from server at: " + url, e); + } catch (SolrException se) { + throw se; + } catch (URISyntaxException | RuntimeException re) { + throw new SolrServerException(re); + } finally { + // See + // https://docs.oracle.com/en/java/javase/17/docs/api/java.net.http/java/net/http/HttpResponse.BodySubscribers.html#ofInputStream() + if (!wantStream(parser)) { + try { + resp.body().close(); + } catch (Exception e1) { + // ignore + } + } + } + } + + private HttpResponse<InputStream> doGet( + String url, + HttpRequest.Builder reqb, + SolrRequest<?> solrRequest, + ModifiableSolrParams queryParams) + throws IOException, InterruptedException, URISyntaxException { + validateGetRequest(solrRequest); + reqb.GET(); + decorateRequest(reqb, solrRequest); + reqb.uri(new URI(url + "?" + queryParams)); + return client.send(reqb.build(), HttpResponse.BodyHandlers.ofInputStream()); + } + + private HttpResponse<InputStream> doPutOrPost( + String url, + boolean isPut, Review Comment: instead of a boolean, can you use the enum reference? Boolean args are not very clear. And this would allow you to combine both case statements back in the switch. -- 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
