scijava / scijava-common

A plugin framework and application container with built-in extensibility mechanism :electric_plug:
BSD 2-Clause "Simplified" License
87 stars 52 forks source link

Сontext creates extra thread that prevents normal closing application #458

Closed Daniel-Alievsky closed 1 year ago

Daniel-Alievsky commented 1 year ago

I already created an issue in SCIFIO project: https://github.com/scifio/scifio/issues/486 But probably it should be here.

I'm using SCIFIO for reading and parsing large TIFF files with help of your TiffParser class.

I've notices that almost any usage of io.scif SCIFIO (current version 0.44) creates a background non-daemon thread, named like SciJava-6b53e23f-Thread-0@1742

This is a problem: this thread prevents an application to be normally closed.

This thread appears while any attempt to create SCIFIO context, both by new Context(false) or SCIFIO scifio = new SCIFIO(); Context context = scifio.getContext(); operators. The only variant when there is no problem is creating empty context by new Context(true), but such a context is useless: it leads to NullPointerException when passed to TiffParser constructor.

I've found a possible workaround: I can manually close the created context by context.close() call, and the extra thread will be closed. However, it is not a good solution due to at least 2 reasons. 1) Contexts are created in invisible manner by a lot of SCIFIO methods, like IO.opener(), IO.save(some_image) etc. If at least one of such contexts will not be closed manually, parasite thread remains. 2) I'm using SCIFIO as a maven library A, which is used inside another library B, which is used inside another library C, etc. My end user does not know anything about the fact that a low-level library A uses SCIFIO, when the processed file is TIFF - it is an implementation detail. So, at the high level there is no suitable point where we could close SCIFIO contexts.

Is it possible to resolve this problem? In particular, is it possible to make the service thread "SciJava-..." a daemon thread, not preventing normal usage of this library? Or, maybe, you could add an ability to create working context without creating any threads?

Note that this problem was not so critical in old SCIFIO version 0.11.0. In that version, the call SCIFIO scifio = new SCIFIO() did not create an additional thread, and we could freely use scifio.getContext() But calls like IO.openImgs(...) were problematic also in that version.

ctrueden commented 1 year ago

A minimal example to reproduce:

public static void main(String[] args) throws Exception {
        Context context = new Context();
//        context.close();
        System.out.println("Done");
    }

and it hangs, because the ExecutorService (created by Executors.newCachedThreadPool) spins out non-daemon threads by default, which sit around waiting for new jobs, blocking JVM shutdown, until the ExecutorService is shut down.

Unfortunately, just starting up the Context seems to result in calls to threadService.run, I believe via eventService.publishLater, which is why the simple example above hangs.

Changing to daemon threads is not sufficient though, because we do not want to allow JVM shutdown while these threads are still actively doing work. The Guava library handles this pitfall by adding a shutdown hook that waits for the daemon threads to finish pending work. I will try to implement that solution here in SciJava Common.

ctrueden commented 1 year ago

@Daniel-Alievsky I implemented it. In theory, you shouldn't have to call context.dispose() manually anymore. At least, the above minimal example now terminates immediately without hanging. Let me know how it works for you.

Daniel-Alievsky commented 1 year ago

How can I test it?

1) I've updated scijava-common (master branch) and created in "test/java" folder a simplest test:

package org.tests;

import org.scijava.Context;

public class SimplestContext {
    public static void main(String[] args) {
        Context context = new Context();
//        context.close();
        System.out.println("Done");
    }
}

It does not run at all:

[WARNING] Class pool is empty: forgot to call Thread#setClassLoader? Exception in thread "main" java.lang.IllegalArgumentException: No compatible service: org.scijava.service.Service at org.scijava.service.ServiceHelper.loadService(ServiceHelper.java:241) at org.scijava.service.ServiceHelper.loadService(ServiceHelper.java:192) at org.scijava.service.ServiceHelper.loadServices(ServiceHelper.java:168) at org.scijava.Context.(Context.java:291) at org.scijava.Context.(Context.java:240) at org.scijava.Context.(Context.java:136) at org.scijava.Context.(Context.java:123) at org.scijava.Context.(Context.java:112) at org.tests.SimplestContext.main(SimplestContext.java:7)

2) I've tried to install this (mvn install). It failed:

[INFO] Running org.scijava.widget.WidgetStyleTest [INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.006 s - in org.scijava.widget.WidgetStyleTest [INFO] [INFO] Results: [INFO] [ERROR] Failures: [ERROR] ObjectIndexTest.testToString:220 array lengths differed, expected.length=7 actual.length=9; arrays first differed at element [6]; expected:<[org.scijava.object.ObjectIndex$All]: {5, 2.5, 3}> but was:<[ java.lang.constant.Constable]: {5, 2.5, 3}> [ERROR] Errors: [ERROR] BytesHandleTest>DataHandleTest.testWriting:127->DataHandleTest.checkAdvancedStringWriting:385->DataHandleTest.checkAdvancedStringWriting:408 ┬╗ InaccessibleObject [ERROR] WriteBufferDataHandleTest.testEndiannessWriting:108->DataHandleTest.checkAdvancedStringWriting:408 ┬╗ InaccessibleObject [ERROR] DigestUtilsTest.testBase64:95 ┬╗ NoClassDefFound javax/xml/bind/DatatypeConvert... [ERROR] DigestUtilsTest.testBestBase64Bytes:186 ┬╗ NoClassDefFound javax/xml/bind/Datat... [ERROR] DigestUtilsTest.testBestBase64String:179 ┬╗ NoClassDefFound javax/xml/bind/Data... [INFO] [ERROR] Tests run: 792, Failures: 1, Errors: 5, Skipped: 3 [INFO] [INFO] ------------------------------------------------------------------------ [INFO] BUILD FAILURE [INFO] ------------------------------------------------------------------------ [INFO] Total time: 10.631 s [INFO] Finished at: 2023-05-03T21:05:39+03:00 [INFO] ------------------------------------------------------------------------ [ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.22.2:test (default-test) on project scijava-common: There are test failures. [ERROR] [ERROR] Please refer to C:\W\scijava-common\target\surefire-reports for the individual test results. [ERROR] Please refer to dump files (if any exist) [date].dump, [date]-jvmRun[N].dump and [date].dumpstream. [ERROR] -> [Help 1] [ERROR] [ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch. [ERROR] Re-run Maven using the -X switch to enable full debug logging. [ERROR] [ERROR] For more information about the errors and possible solutions, please read the following articles: [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoFailureException

3) I've updated scifio, master branch, and run my previous test: Context context = new SCIFIO().getContext();

It still hangs, probably because it still use scijava-common 2.91.0 from maven, not your latest version.

ctrueden commented 1 year ago

@Daniel-Alievsky wrote:

How can I test it?

You can add a dependency to org.scijava:scijava-common:2.92.1-SNAPSHOT in your main project, since the fix is already pushed to the mainline branch and deployed by CI to the maven.scijava.org snapshots repository. Then you should see the improved context cleanup behavior in your application.

Daniel-Alievsky commented 1 year ago

Ok, it seems to be working.

Is it possible to rework contexts so that they will not create threads at all? Why do they require asynchronous threads? Or, at least, maybe it is possible not to create threads, when we need really simple actions, like parsing TIFF in TiffParser class? I remind that old SCIFIO version 0.11 did not create threads in this situation: I could freely use TiffParser (though threads were created while more complex operations).

ctrueden commented 1 year ago

@Daniel-Alievsky The EventService uses threads to publish events via the ThreadService. How would it help you to have these threads not exist? Is it harming the performance or behavior of your application? If not, I would not worry about it.