Using a thread pool for pin state notifications can reorder events

[ylexus]

pi4j uses an internal thread pool to deliver PIN state events. Problem with this approach is that if your software is relying on the latest GpioPinDigitalStateChangeEvent.getState(), then, because the order of events in the thread pool is not guaranteed, your listener can end up having the wrong final event value, if the rate of event change is rapid enough.

Consider this PIN state change sequence:

1 LOW 2 HIGH 3 LOW

The final state is low, but if it so happens that the thread that delivers event (3) takes longer than the time difference between events (2) and (3), and the thread that delivers event (2) is faster, then the listener will see the events in this order:

1 LOW 3 LOW 2 HIGH

So the listener now erroneously thinks that the pin is in high state.

The workaround is to ignore the event value and always query the pin state in the event listener.

I think the idea of using a thread pool for pin state notifications, as opposed to a single thread, is not a good idea.

[eitch]

Hi @ylexus yes i agree with you and i think i have run into this exact problem myself. I shall check it and make a fix on my fork: https://github.com/eitch/pi4j

[ylexus]

Thanks. Don’t think it’s the library’s responsibility to do threading. If a user has heavyweight event handlers that need a thread pool to be executed, it’s the user’s concern, not the library’s. I may be missing something on the design, of course. At the bare minimum there should be one thread per PIN, or, simpler, one thread for all PINs.

[savageautomate]

There is a single native thread spun up to detect events from the underlying system. Then in the Java code these "native" events are delegated to new events pushed into a thread pool to prevent any long operation blocking on the native thread.

Perhaps this thread pool for GPIO event handling on the Java should be a single threaded pool?
I don't remember all the specifics, but the logic is spread across different parts of the project

• https://github.com/Pi4J/pi4j/blob/a4f0c7f00908ae32bffefe834dc21c798575bec3/pi4j-core/src/main/java/com/pi4j/io/gpio/impl/GpioEventMonitorExecutorImpl.java
  • https://github.com/Pi4J/pi4j/blob/master/pi4j-core/src/main/java/com/pi4j/wiringpi/GpioInterrupt.java#L119

It should also be possible to assign your own ExecutorServiceFactory (https://github.com/Pi4J/pi4j/blob/bc8eb2916a360f08f95c9b647df491554990cd4c/pi4j-core/src/main/java/com/pi4j/concurrent/ExecutorServiceFactory.java) via the GpioFactory (https://github.com/Pi4J/pi4j/blob/master/pi4j-core/src/main/java/com/pi4j/io/gpio/GpioFactory.java#L147) and control the executor used for GPIO event handling via your own impl of:

public ExecutorService getGpioEventExecutorService();

[ylexus]

@savageautomate in my view, the event handling pool must indeed be single threaded and there should be no way for users to make it multi-threaded, or at least, if such a way exists (GpioFactory you mentioned) then

1. the default should be singe-threaded
2. the documentation on the override facility should have a warning that multiple threads will lose the event order guarantee.

In my case, global static override via GpioFactory is not an option, as my software is a library. A library does not have a right to override the event handling pool for the whole JVM. I would suggest removing all global static overrides like that and instead provide an instance-bound configuration via GpioFactory, something like that:

GpioFactory gpioFactory = GpioFactory.builder()
    .executorServiceFactory(myExecutorServiceFactory)
    .build()

[eitch]

@savageautomate I have to agree with @ylexus . I ran into the same problem where interrupts for an I2C bus came in the wrong order and messed things up. I think this is something that should be handled on the client side, if a ordering guarantee is not needed and interrupt event handlers take their dear time.

[savageautomate]

@eitch & @ylexus

I agree with both of you in that a single threaded model should be used to ensure event order. No question, we should fix this for V2. So that leaves the question, should this be changed for the upcoming 1.4 release?

[eitch]

@savageautomate I think we should change it for the 1.4 release as it has bad side affects. I will create a patch asap.

[savageautomate]

@eitch

OK, so you are taking on this change then?

Thanks, Robert

[savageautomate]

@ylexus ,

In my case, global static override via GpioFactory is not an option, as my software is a library. A library does not have a right to override the event handling pool for the whole JVM. I would suggest removing all global static overrides like that and instead provide an instance-bound configuration via GpioFactory, something like that:

GpioFactory gpioFactory = GpioFactory.builder()
    .executorServiceFactory(myExecutorServiceFactory)
    .build()

I think you will like the patterns we have in place for Pi4J V2. No static singletons. A Pi4J Context object is created (via builder pattern) and can be customized by the user for its runtime. All Pi4J references, lifecycles, threads, etc are all managed inside the context.

• https://v2.pi4j.com/documentation/context
• https://v2.pi4j.com/documentation/architecture

Here is an example/demo project: https://github.com/Pi4J/pi4j-demo-telegraph/blob/master/src/main/java/com/pi4j/demo/telegraph/Telegraph.java

This demo code also shows that I/O instances are created using builders. Lots of good stuff on the way ....

[eitch]

@savageautomate yes, i will implement this change for 1.4 asap

[savageautomate]

@eitch

Just be aware that there are multiple tasks (pin pulse, blink, events, debouncing, etc) related to GPIO pins using the scheduled executor service.

https://github.com/Pi4J/pi4j/tree/master/pi4j-core/src/main/java/com/pi4j/io/gpio/tasks/impl

Thanks, Robert

[eitch]

I have now added a single threaded GPIO executor service on my branch at https://github.com/eitch/pi4j/tree/develop/1.4

One can then activate it like so:

import com.pi4j.concurrent.SingleThreadGpioExecutorServiceFactory;
...
GpioFactory.setExecutorServiceFactory(new SingleThreadGpioExecutorServiceFactory());

Maybe pi4j wants to use it by default. I tested it in one of our applications, of course it should be done before using the GpioFactory.

[eitch]

I think this issue can be closed as the SingleThreadGpioExecutorServiceFactory solves this.