Alright; thanks for your thorough response. If you/someone notices a
failure and we need to touch this again, let's follow up with a PR.
And I suspect we'll want to factor out the cleanup so more tests might
use this without the copy-pasted hack. Perhaps HttpJdkSolrClient
could do this cleanup in its close instead of the test? Also I
thought I suggested before checking if httpClient implements
AutoCloseable in HttpJdkSolrClient.close() so you can call it. This
would work nicely when the user chooses to run in JDK 21.
On Fri, Mar 15, 2024 at 9:59 AM James Dyer <jdyer....@gmail.com> wrote:
>
> David, let me try and address your concerns with the approach. I am
> open to other approaches, however, we need something that will work
> consistently. I would like for what I have now to run in our CI for
> at least a week to see if we get more failures or not, before changing
> it further. I do not want to contribute to this project's flakey test
> problem.
>
> As the wisdom of the JDK designers had it, java.net.http.HttpClient
> did not originally implement Closable or Autoclosable. There was no
> way for a user of the library to release resources, particularly
> threads. You can pass your own Executor in the Builder, keep a
> reference to it and ensure you close it yourself. But this is not
> enough. The implementation appears to keep additional ("selector")
> threads not managed by that executor.
>
> See the description of [1] and the last paragraph of [2], where it is
> noted that prior to Java 21 (which adds close() methods) we must wait
> for a garbage collection. While developing this feature, I found that
> running a full gc in the "after" method of each test was enough to
> make the test framework happy. However, within a day of this being in
> our CI, we were getting failures from these "selector" threads still
> being active. As I said, I do not want to contribute further to our
> flakey test problem.
>
> Others suggest similar solutions in [3], although the reflection-based
> solutions are less-viable because they I believe require "--add-opens"
> to be added for the module, which I think we should avoid. I agree
> the current solution of interrupting all the "HttpClient" threads in
> the "after" method is an extraordinary step to take! It probably
> would be adequate to do it in "afterClass" but then again, it might be
> best if each test started with a clean slate. If we join the threads
> first, the test might block waiting. I am pretty sure the "system.gc"
> is no longer necessary after interrupting the threads, but in a unit
> test "after" method, what is the harm?
>
> I agree I should have used "forEach". I noticed this myself after
> pushing these changes. But this is the most minor of concerns.
> -----
> [1] https://bugs.openjdk.org/browse/JDK-8304165
> [2] https://inside.java/2023/12/10/sip092/
> [3] https://stackoverflow.com/questions/53919721/close-java-http-client
>
> On Thu, Mar 14, 2024 at 8:35 PM dsmiley (via GitHub) <g...@apache.org> wrote:
> >
> >
> > dsmiley commented on PR #2259:
> > URL: https://github.com/apache/solr/pull/2259#issuecomment-1998742427
> >
> > Instead of simply interrupting the threads, shouldn't we also join on
> > them?
> > Should that be in an AfterClass -- is it a problem between each test?
> > note: you don't need to gather a Set to then iterate; you can call
> > `forEach` on the stream to do whatever you want to do. I don't think using
> > a Set (for uniqueness) is necessary here either.
> > What is the GC accomplishing?
> >
> > I was looking again at all those test overrides that call super. I
> > removed them locally and I was able to run at the CLI and IntelliJ and it
> > ran all the intherited tests. This pattern is done elsewhere like
> > SolrExampleTests
> >
> >
> > --
> > 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
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org
> For additional commands, e-mail: dev-h...@solr.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@solr.apache.org
For additional commands, e-mail: dev-h...@solr.apache.org
