MangoAutomation / BACnet4J

BACnet/IP stack written in Java. Forked from http://sourceforge.net/projects/bacnet4j/
GNU General Public License v3.0
183 stars 110 forks source link

Comparing the timeout against the request time. #87

Closed DangR-x closed 11 months ago

DangR-x commented 11 months ago

The line long now = localDevice.getClock().millis(); in the expire() method of DefaultTransport class retrieves the system time when the local device was initialized. However, this time is used to determine if a request has timed out. The proper comparison should be made when invoking localDevice.send(remoteDevice, multipleRequest) image

terrypacker commented 11 months ago

Thanks for the suggestion but I'm not quite following what you are saying. Can you provide a code sample or better yet a PR to show the changes you want made?

DangR-x commented 11 months ago

The private field Clock clock = Clock.systemUTC() in the LocalDevice class is assigned during the initialization of LocalDevice. When LocalDevice.send is called, the request's timeout is verified through DefaultTransport.expire(). The condition for determining timeout is the comparison between the current request initiation time and LocalDevice.getClock(). However, if the current requirement is to initialize LocalDevice and initiate a request after a significant time has passed, the timeout validation will never be triggered.

terrypacker commented 11 months ago

Ok I see what you are saying. I think one of us is misunderstanding what this line is doing:

        final long now = localDevice.getClock().millis();

I believe it should return the system time at the moment the method is called. Not the system time when the local device was initialized and got a reference to that clock.

So I don't think there is a problem with using that call vs. whenever the local device is initialized.

terrypacker commented 11 months ago

The Javadoc from java.time.Clock

    /**
     * Obtains a clock that returns the current instant using the best available
     * system clock, converting to date and time using the UTC time-zone.
     * <p>
     * This clock, rather than {@link #systemDefaultZone()}, should be used when
     * you need the current instant without the date or time.
     * <p>
     * This clock is based on the best available system clock.
     * This may use {@link System#currentTimeMillis()}, or a higher resolution
     * clock if one is available.
     * <p>
     * Conversion from instant to date or time uses the {@linkplain ZoneOffset#UTC UTC time-zone}.
     * <p>
     * The returned implementation is immutable, thread-safe and {@code Serializable}.
     * It is equivalent to {@code system(ZoneOffset.UTC)}.
     *
     * @return a clock that uses the best available system clock in the UTC zone, not null
     */
    public static Clock systemUTC() {
        return SystemClock.UTC;
    }

If you look at the implementation the call to millis() on the SystemClock class it does this:

        @Override
        public long millis() {
            // inline of SystemInstantSource.INSTANCE.millis()
            return System.currentTimeMillis();
        }
terrypacker commented 11 months ago

If you agree please close this issue. If not then convince me otherwise.