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
