SOLR-18312: introduce dedicated thread pool executor for httpClientBu…

[renatoh]

see ticket https://issues.apache.org/jira/browse/SOLR-18312

[Copilot]

Pull request overview

This PR (SOLR-18312) updates HttpJdkSolrClient to use a dedicated executor for the JDK HttpClient, aiming to prevent deadlocks when request bodies are streamed (producer) while the JDK client consumes them (consumer), particularly under HTTP/1.1 with concurrent requests.

Changes:

• Introduce a separate httpClientExecutor that is always owned and shut down by HttpJdkSolrClient, instead of reusing the request-body streaming executor.
• Add a concurrency regression test that issues multiple concurrent JsonQueryRequest calls with large streamed bodies while forcing HTTP/1.1.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpJdkSolrClient.java	Adds a dedicated executor for the underlying JDK HttpClient and ensures it is shut down on close.
solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpJdkSolrClientTest.java	Adds a concurrent, streamed-body test intended to catch deadlocks under HTTP/1.1.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[dsmiley]

I think it'd be more natural for the builder/configured executor to be for the httpClient usage itself, just as the jetty one.

[dsmiley]

I pushed an improvement to defer the use of the executor for body thread pushing to only start when it's requested (by the supplier). Maybe in practice won't be notice but I like the code for it much better now. CC @serhiy-bzhezytskyy

[serhiy-bzhezytskyy]

@dsmiley the deferral does more than tidy the code — it removes the leak by construction for one whole class of failure. I measured it with a counting executor on a refused connection: before e534b380e one body task is submitted and then has to be released; after it, zero (expected:<0> but was:<1> against the previous commit). So the original SOLR-17707 guard now only covers "body requested, then the connection dropped", which is the narrower case it should have been.

Two things I checked while I was in here, both fine:

• the mechanism sentence still holds. I mutated the fix on main to keep only cancel(true) and the SOLR-17707 test fails with content-writing thread leaked, still blocked after failure — so close(sink) is still load-bearing, and moving the comment along with the code was right.
• contentWritingSink is assigned before contentWritingFuture, so a releaseContentWriting() arriving between them finds a null future and an already-closed sink. Over 200 runs that is 200 writers exiting on IOException with the pool fully drained — log noise, not a leak. Not worth a change.

The one thing missing is a test for the new property. I have the one above if you want it, against this branch or as a follow-up.

[dsmiley]

Thanks for the review Serhiy. testStuckContentWritingThreadIsReleasedOnFailure that you added previously is good enough IMO. I just pushed a change to switch from volatile to synchronized, which avoids the sequence issue you pointed out -- thanks for that.

[renatoh]

@dsmiley I resolved all the Copilot remarks. As far as I can see everything has been resolved, or is there anything left before this PR can be merged?

[dsmiley]

Also the change (that I introduced RE executor initialization) results in no enforced cap/limit on threads. I didn't check if there's one at the JDK HttpClient level. If there's no limit, then it's worth a major-changes-since adoc file note. We should probably have a bit of javadoc somewhere to communicate whatever limit may or may not exist so at least users know. If there's no limit, IMO it's fine. It's straight-forward for a user to enforce via a semaphore if they wish.

[renatoh]

Also the change (that I introduced RE executor initialization) results in no enforced cap/limit on threads. I didn't check if there's one at the JDK HttpClient level. If there's no limit, then it's worth a major-changes-since adoc file note. We should probably have a bit of javadoc somewhere to communicate whatever limit may or may not exist so at least users know. If there's no limit, IMO it's fine. It's straight-forward for a user to enforce via a semaphore if they wish.

@dsmiley
I could not find anything regarding a limit, and I was able to get the thread pool up to 8k threads in some local testing.
I have added some documentation. I've noticed for the HttpJdkSolrClient.executor, we have an attribute on the builder, the user can pass in his own executor if he needs different settings. But we do not have that for httpClientExecutor. I think it would make sense to add it to the builder, what do you think?

[dsmiley]

But we do not have that for httpClientExecutor.

Maybe I wasn't clear in my last message but I meant to communicate that IMO the executor on the builder should be for HttpClient. It is before this PR; you changed it in this PR. I think you should please change it back. I don't think a user should need to customize the executor used for POST'ing.

[renatoh]

@dsmiley OK, now I got it. Next try;)

[dsmiley]

missing a changelog

[renatoh]

missing a changelog
What is the criteria met here for a changelog? I would have thought such a fix is not something we add to the changelog.

[dsmiley]

You found a real bug and inspired you to fix it (captured in the JIRA issue); someone else is bound to hit this bug sooner or later. So definitely deserves a changelog!

[renatoh]

You found a real bug and inspired you to fix it (captured in the JIRA issue); someone else is bound to hit this bug sooner or later. So definitely deserves a changelog!

Makes sense, thanks for your support in fixing this!