Conditionally wait for previous polling task

[vpaturet]

Summary

This is a follow-up PR for #6262 that improves the blocking wait logic: the polling thread is blocked only if the previous task takes longer than the polling interval.
In this case a log message indicates the additional waiting period.

The encapsulation of the task submission to the graph writer thread was missing in the SIRI-SX and SIRI-ET updater, this is fixed. As a result the callback saveResultOnGraph can be made private in PollingGraphUpdater.

The logic covers also the case when an updater sends several update tasks during one polling cycle.

Issue

No

Unit tests

The tests added in #6262 do not apply anymore and are removed.
Unit-testing the correctness of the logic introduced in this PR is impractical (multi-threading), so unfortunately no unit test is added.
Manual testing with the SIRI-SX polling updater in a local environment shows that the code works as expected.

Documentation

No

[codecov]

Codecov Report

Attention: Patch coverage is 25.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 69.72%. Comparing base (5ab75af) to head (55aae1f).
Report is 68 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...entripplanner/updater/spi/PollingGraphUpdater.java	30.00%	5 Missing and 2 partials ⚠️
...ripplanner/updater/siri/updater/SiriETUpdater.java	0.00%	1 Missing ⚠️
...ripplanner/updater/siri/updater/SiriSXUpdater.java	0.00%	1 Missing ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6401      +/-   ##
=============================================
- Coverage      69.73%   69.72%   -0.01%     
+ Complexity     18023    18022       -1     
=============================================
  Files           2057     2057              
  Lines          76978    76985       +7     
  Branches        7845     7844       -1     
=============================================
- Hits           53678    53677       -1     
- Misses         20550    20555       +5     
- Partials        2750     2753       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[t2gran]

I looked at this a second time because it suddenly appeared to me that it maybe would be a threading issue here.

[t2gran]

Add @nullable here.

[t2gran]

If this is non-blocking, does that mean it can be called from multiple threads or from one thread only? This should be documented in the JavaDoc.

What prevent this from waiting on it self?

I was currious what would happen if you try waiting on your self? The Future#get does not say, but this small program hangs - never returns:

  public static class A {
    Future previous = null;

    public static void main(String[] args) throws Exception {
      var pool = Executors.newFixedThreadPool(5);
      var a = new A();
      var f = pool.submit(a::run);
      a.previous = f;
    }

    void run() {
      try {
        Thread.sleep(500);
        Thread.yield();
        if (previous != null) {
          System.err.println("Reference to it self obtained!");
          previous.get();
          System.err.println("Do not get here!");
        }
      } catch (Exception e) {
        throw new RuntimeException(e);
      }
    }
  }

Output:

Reference to it self obtained!

[abyrd]

@t2gran my understanding is that this method is intended to only be called in the runPolling() implementations of PollingGraphUpdater subclasses. Those are in turn only expected to be called by PollingGraphUpdater.run() which is in turn only run by GraphUpdaterManager.pollingUpdaterPool.scheduleWithFixedDelay() which, if I'm not mistaken, means it would only ever be called on one thread at a time.

So I think the self-waiting situation you describe should only arise if someone made a PollingGraphUpdater subclass, on which the runPolling method was implemented, written to contain something like updateGraph(self) instead of updateGraph(closure). Such intentional indirect recursion would imply the author was confused about how these updaters are meant to work. Would Javadoc clearly stating how this method is used in PollingGraphUpdaters be sufficient?

[abyrd]

Added a few comments for now which are more about documentation than the functionality itself, which seems sound. I am happy to update/add the Javadoc myself if you can confirm whether I'm understanding things correctly.

[abyrd]

This is not a new change, but it still seems a bit odd to me that the interface and field have different names here, with the field name being a verb. There's also a comment below referring to this as the "GraphWriter" and I'm not sure anything still has that name. Probably all a result of a series of renamings as the callback interface was extracted from the update manager.

[abyrd]

I don't think the expression "handle on" is idiomatic, particularly without an article. It might be mistaken for an imperative verb. I suggest we adapt the language in ExecutorService.submit Javadoc: "a Future representing pending completion of the task".

Suggested change

	* Handle on the task posted during the previous polling execution.
	* A Future representing pending completion of most recently submitted task.

[abyrd]

The system in this PR where this method is called from runPolling() in all subclasses, with this method being the sole way to submit tasks to a private WriteToGraphCallback seems like an even better approach than what I was speculating about in #6262 (comment).
I'd recommend we briefly explain that usage pattern in the Javadoc here though.

[abyrd]

@t2gran my understanding is that this method is intended to only be called in the runPolling() implementations of PollingGraphUpdater subclasses. Those are in turn only expected to be called by PollingGraphUpdater.run() which is in turn only run by GraphUpdaterManager.pollingUpdaterPool.scheduleWithFixedDelay() which, if I'm not mistaken, means it would only ever be called on one thread at a time.

So I think the self-waiting situation you describe should only arise if someone made a PollingGraphUpdater subclass, on which the runPolling method was implemented, written to contain something like updateGraph(self) instead of updateGraph(closure). Such intentional indirect recursion would imply the author was confused about how these updaters are meant to work. Would Javadoc clearly stating how this method is used in PollingGraphUpdaters be sufficient?