Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Presence of a non-optional shutdown hook messes up with shutdown sequence #371

Closed
ylexus opened this issue Jul 29, 2024 · 9 comments
Closed

Comments

@ylexus
Copy link

ylexus commented Jul 29, 2024

Related somewhat to #213. I have an application that properly manages component dependencies, including the order of initialisation and shutdown. pi4j context is just another component in the app that is initialised before it's used by other components, and is shut down via Context.shutdown() (which shuts down the Runtime) just after all the components that use it have already shut down.

However, DefaultRuntime adds a shutdown hook that is executed concurrently with my application shutdown sequence and obliterates the context before the components that use pi4j are stopped, causing exceptions (example is below, but it does not matter).

I think a component based library that has clear start/stop points (which pi4j does) should not unconditionally start shutting itself down. I agree it's useful for apps don't care about shutdown sequences or exceptions, but for more organised apps like mine it would be very useful to make the shutdown hook optional.

I can raise a PR, possibly by adding a property that disables it so that a user could do:

newContextBuilder().autoDetect().property("noShutdownHook", "true").build()

, but please recommend the best way of doing this.

2024-07-29T18:57:10,352 ERROR [Thread-8] n.y.a.d.p.Ads1115 Uncaught exception in continuous reading thread
com.pi4j.exception.Pi4JException: Failed to execute action for device 72 on bus 1
        at com.pi4j.io.i2c.I2CBusBase._execute(I2CBusBase.java:57) ~[automator.jar:?]
        at com.pi4j.plugin.linuxfs.provider.i2c.LinuxFsI2CBus.executeIOCTL(LinuxFsI2CBus.java:84) ~[automator.jar:?]
        at com.pi4j.plugin.linuxfs.provider.i2c.LinuxFsI2C.readRegister(LinuxFsI2C.java:311) ~[automator.jar:?]
        at com.pi4j.plugin.linuxfs.provider.i2c.LinuxFsI2C.readRegister(LinuxFsI2C.java:173) ~[automator.jar:?]
        at com.pi4j.io.i2c.I2CRegisterDataReader.readRegister(I2CRegisterDataReader.java:206) ~[automator.jar:?]
        at com.pi4j.io.i2c.I2CRegisterDataReader.readRegister(I2CRegisterDataReader.java:220) ~[automator.jar:?]
        at com.pi4j.io.i2c.I2CRegisterDataReader.readRegisterWord(I2CRegisterDataReader.java:706) ~[automator.jar:?]
        at net.yudichev.automator.devices.pi4j.I2CDevice.readRegister(I2CDevice.java:43) ~[automator.jar:?]
        at net.yudichev.automator.devices.pi4j.Ads1115.readSingleValue(Ads1115.java:199) ~[automator.jar:?]
        at net.yudichev.automator.devices.pi4j.Ads1115.lambda$readAllChannels$0(Ads1115.java:228) ~[automator.jar:?]
        at java.util.HashMap.forEach(HashMap.java:1429) ~[?:?]
        at net.yudichev.automator.devices.pi4j.Ads1115.lambda$readAllChannels$1(Ads1115.java:226) ~[automator.jar:?]
        at java.lang.Thread.run(Thread.java:1583) [?:?]
Caused by: com.pi4j.exception.Pi4JException: Failed to execute ioctl for device 72 on bus 1
        at com.pi4j.plugin.linuxfs.provider.i2c.LinuxFsI2CBus.lambda$executeIOCTL$2(LinuxFsI2CBus.java:91) ~[automator.jar:?]
        at com.pi4j.io.i2c.I2CBusBase._execute(I2CBusBase.java:44) ~[automator.jar:?]
        ... 12 more
Caused by: java.io.IOException: failed to get POSIX file descriptor!
        at com.pi4j.library.linuxfs.LinuxFile.getPosixFD(LinuxFile.java:197) ~[automator.jar:?]
        at com.pi4j.library.linuxfs.LinuxFile.ioctl(LinuxFile.java:167) ~[automator.jar:?]
        at com.pi4j.plugin.linuxfs.provider.i2c.LinuxFsI2CBus.lambda$executeIOCTL$2(LinuxFsI2CBus.java:87) ~[automator.jar:?]
        at com.pi4j.io.i2c.I2CBusBase._execute(I2CBusBase.java:44) ~[automator.jar:?]
        ... 12 more
@eitch
Copy link
Member

eitch commented Jul 30, 2024

I'm not quite sure I'm following. Pi4j only stops everything, when shutdown is called. Maybe you are just calling it too soon? Do you have a more detailed example of how you are managing pi4j's lifecycle?

@ylexus
Copy link
Author

ylexus commented Jul 30, 2024

@eitch

Pi4j only stops everything, when shutdown is called.

So intuitively it should, but unfortunately this is not the case. As you can see, the DefaultRuntime installs a global JVM shutdown hook and shuts the context down when that hook fires. As a result:

  1. The JVM receives SIGTERM - this initiates JVM shutdown which calls all JVM shutdown hooks in unspecified order.
  2. My application's shutdown hook is fired, application starts stopping its services
  3. At the same time, concurrently and in unspecified order, the pi4j shutdown hook is fired, shutting down the pi4j context.
  4. The application shutdown sequence reaches the service that uses pi4j too late - this service manages to still invoke a few methods like reading from the I2C bus seen in the stack trace I posted. But because (3) has already shut down pi4j, it results in exceptions.
  5. The application shutdown sequence closes all components that use pi4j
  6. The shutdown sequence now calls pi4j context's shutdown(), however it does nothing as it's already been called by (3)

My point is that the global shutdown hook that you can't turn off is not a good idea to have in a library, because the application that uses the library loses full control of the shutdown sequence.

Hope this is clear.

@ylexus
Copy link
Author

ylexus commented Aug 1, 2024

The workaround for this is to register a pi4j shutdown listener and block the pi4j shutdown until all the components that use it have shut down (https://github.com/ylexus/jiotty/blob/master/jiotty-connector-rpi/src/main/java/net/yudichev/jiotty/connector/rpigpio/Pi4jContextProvider.java). It's rather elaborate; disabling pi4j shutdown hook would be a much preferred solution.

final class Pi4jContextProvider extends BaseLifecycleComponent implements Provider<Context> {
    private static final Logger logger = LoggerFactory.getLogger(Pi4jContextProvider.class);
    private static final int SHUTDOWN_TIMEOUT_SECONDS = 15;

    private final CountDownLatch preShutdownLatch = new CountDownLatch(1);
    private final CountDownLatch postShutdownLatch = new CountDownLatch(1);
    private volatile boolean pi4jShuttingDown;
    private Context gpio;

    @Override
    public Context get() {
        return whenStartedAndNotLifecycling(() -> gpio);
    }

    @Override
    public void doStart() {
        gpio = Pi4J.newAutoContext();
        // TODO this is a workaround for https://github.com/Pi4J/pi4j-v2/issues/371 - remove when fixed
        gpio.addListener(new ShutdownListener() {
            @Override
            public void beforeShutdown(ShutdownEvent event) {
                pi4jShuttingDown = true;
                logger.debug("Blocking pi4j shutdown until this component is closed");
                asUnchecked(() -> {
                    var stopTriggered = preShutdownLatch.await(SHUTDOWN_TIMEOUT_SECONDS, SECONDS);
                    if (stopTriggered) {
                        logger.debug("Stopping - pi4j shutdown released");
                    } else {
                        logger.warn("Timed out waiting for this component to be stopped");
                    }
                });
            }

            @Override
            public void onShutdown(ShutdownEvent event) {
                postShutdownLatch.countDown();
            }
        });
    }

    @Override
    protected void doStop() {
        logger.debug("Releasing pi4j shutdown gate and shutting down pi4j");
        preShutdownLatch.countDown();
        // best effort prevent double shutdown call and awaiting successful shutdown, will work if the pi4j shutdown hook fires first
        if (pi4jShuttingDown) {
            // pi4's own shutdown hook is doing the job - wait for shutdown to finish
            logger.debug("Waiting for pi4j to shut down");
            asUnchecked(() -> {
                var stopped = postShutdownLatch.await(SHUTDOWN_TIMEOUT_SECONDS, SECONDS);
                if (stopped) {
                    logger.debug("pi4j shut down");
                } else {
                    logger.warn("Timed out waiting for pi4 to shut down");
                }
            });
        } else {
            gpio.shutdown(); // does it synchronously
        }
    }
}

@eitch
Copy link
Member

eitch commented Aug 2, 2024

Hi @ylexus

How about this: #372

You can now simply disable the shutdown hook using this:

Pi4J.newContextBuilder().autoDetect().disableShutdownHook().build();

@eitch
Copy link
Member

eitch commented Aug 2, 2024

I never really realized, that pi4j add this shutdown hook, all my test apps, which are simple console apps, etc. add their own shutdown hook to shutdown pi4j. Apps should really control the life cycle, not the library, i totally agree.

@FDelporte
Copy link
Member

not the library

what should be the default then?

@ylexus
Copy link
Author

ylexus commented Aug 2, 2024

Hi @ylexus

How about this: #372

You can now simply disable the shutdown hook using this:

Pi4J.newContextBuilder().autoDetect().disableShutdownHook().build();

Yes that would totally work, thanks. This is exactly what I asked.

@eitch
Copy link
Member

eitch commented Aug 5, 2024

Fixed in next release

@eitch eitch closed this as completed Aug 5, 2024
@ylexus
Copy link
Author

ylexus commented Sep 28, 2024

@eitch been a long time without releases - time to do one maybe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants