Any objections to integrating this?

This is a bit raw patch, I expect you still want to work on other attributes of acceptable API patch. So just one general comment:

Y01 Align more with java.util.concurent. 

Rather then inventing new methods and method names, keep consistency with one of already available JDK interfaces. The best option seems to make RequestProcessor implement http://java.sun.com/javase/6/docs/api/java/util/concurrent/ScheduledExecutorService.html
It is more work, but it will make the API easier to absorb for regular Java developers.

Agreed with Y01.


[JG01] Odd use of bitwise &, perhaps you meant &&.

Created attachment 94268 [details]
Implementing ScheduledExecutionService over RequestProcessor

Agreed.  It was a fun way to spend a day :-)

Some tests may be somewhat timing dependent.  I have marked the obvious ones as @RandomlyFails - they are still useful during development.  If problems appear in others (they have only been run on one machine), the values may be able to be adjusted - in particular, testScheduleRepeatingIntervalsAreRoughlyCorrect and testScheduleFixedRateAreRoughlyCorrect test that if you specify a repeating task to run every 2 seconds and let it run for a while, that it never is run more frequently than 1.6 seconds.  If this proves unstable, it could be adjusted to, say, 20 seconds and 16 seconds, and it should be fairly rare for that to fail.  Such tests could also be made self-calibrating by measuring the standard deviation of timing for java.util.Timer in setUpClass().

At any rate, everything has tests, and seems to do what the javadoc for ExecutionService says.

@JG01:  Bitwise & use is intentional - both java.util.concurrent.FutureTask.cancel() and RequestProcessor.Task.cancel() need to be called - if I used &&, the second would not be invoked if the first returned true.

(In reply to comment #5)
> Bitwise & use is intentional - both
> java.util.concurrent.FutureTask.cancel() and RequestProcessor.Task.cancel()
> need to be called

OK, but this is such an unusual idiom it would be better to write it explicitly for the benefit of future generations:

boolean canceled1 = ...;
boolean canceled2 = ...; // need to run both!
return canceled1 && canceled2;


[JG02] "implements Executor, ScheduledExecutorService" is redundant.


[JG03] Learn to use @link and @linkplain, e.g.

* is called on the {@linkplain #getDefault default request processor},


[JG04] Class Javadoc should say that SES is implemented since 8.2.

(BTW we usually include the full module code name in @since tags, e.g. "@since org.openide.util 8.2", to avoid misleading comments after refactoring modules.)


[JG05] Use @throws idiomatically:

* @throws IllegalStateException if called on the {@linkplain #getDefault default request processor}


[JG06] Use {@inheritDoc} rather than "...in accordance with..." when implementing an interface if you wish to just add information to the specification (such as a special @throws clause, a note about Cancellable, or other deviations from normal behavior). In this case omit any text or tags which the super Javadoc already includes. And since the class Javadoc should note when SES has been implemented, it is not necessary to mark methods like isShutdown with any Javadoc comment at all: they merely implement the specification.


[JG07] Rather than

  RequestProcessor.logger().setLevel(Level.ALL);

you can use logLevel() to display log messages iff a test fails. That means passing tests are quiet, as they should be.


[JG08] Consider just appending to the existing RequestProcessorTest. It is natural when making an edit in RequestProcessor.java to just press Ctrl-F6 and expect that the right tests will be run. (I suppose apisupport could try to find all tests that look related to the testable code and run them all, but currently it does not.)

Re. JG04 btw. - I don't include module FQN and it never caused a problem. Just make sure when splitting a module to make both module versions higher than the original one.

Re. JG08 - I am fine to have the tests separated. They separate what was in the original RP and the new SES. If you need Ctrl-F6, then probably define pu. st. Test suite() { .... } that will run both tests (of course, rename the new file to not end with Test to prevent double run).

Well, if nearly all the only complaints are about the Javadoc, I'm happy ;-)

> unusual idiom 

Huh, I suppose it is, if you think of it as a bit operation when dealing one-bit values...for that case I always simply thought of it as a way to force full evaluation.  But I suppose it would be clearer.

> [JG07] Rather than
>  RequestProcessor.logger().setLevel(Level.ALL);
> you can use logLevel() to display log messages if a test fails. That means
> passing tests are quiet, as they should be.

So *that's* what JUnit thinks it is doing which it horrifically mis-orders and mangles test output.  How I would love to be liberated from the still-useless JUnit window and just have normal, sequential output that shows up as it goes...

Anyway, I was never able to actually get any logging out of RequestProcessor.logger() even with setting it explicitly.  Didn't find out why, although it might have saved some time.


Re separating the tests:  It's fairly easy to create a request processor in a test and forget to shut it down, causing the JUnit process not to exit.  So it was easier to keep them all out of each others way during test development so new tests couldn't affect old tests.  I don't feel strongly about it either way.

Anyway, easy enough to clean up the javadoc.

(In reply to comment #8)
> I was never able to actually get any logging out of
> RequestProcessor.logger() even with setting it explicitly.

OK, so remove the call to setLevel.

> It's fairly easy to create a request processor in a
> test and forget to shut it down, causing the JUnit process not to exit.  So it
> was easier to keep them all out of each others way during test development so
> new tests couldn't affect old tests.

In this case it is better to keep the new tests in a separate class, since our harness implicitly guarantees that each suite gets its own JVM.

One trivial potential race condition I can imagine couldn't figure out how to test without polluting RequestProcessor with a lock provided by a test - any ideas?

Theoretically, one thread can call stop() (or shutdown() with the patch).  Concurrent with that, another thread can post a new task, before the other thread enters processorLock to stop the RP.  The first thread calls awaitTerminated() - this will block until all threads exit.  However, it cannot hold processorLock while it waits - it's calling Thread.join() on the still-alive threads, and that will block the remaining threads from being returned to the pool.  So it makes a copy of the current threads, and iterates that - which can be a copy without the newly active thread (possible fix would be to repeat that process until everything exits).  So it is possible for awaitTerminated() to return before all threads have really exited (the stopped flag is useless since it indicates pending shutdown, not completed shutdown).

May be too trivial to worry about, as the last thread will eventually exit.

But to write a test for it, I need to be able to block RP.stop() before it starts its dequeueing process, post the new runnable, and then release it, to reproduce this condition.  So far I have not changed any of the logic of the existing RP code, except to add a private Task.cancel() method that can interrupt the RP thread even if the RP says not to, to fulfil the contract of Future.cancel(true).  Any clever tricks?

One other thing to think about:  Two features of Task are not supported by ScheduledExecutor:
 - Ability to reschedule an already run Future - i.e. define 
interface ReschedulableFuture<T> extends Future<T> {
    void schedule (long timeout, TimeUnit unit);
}
and appropriate methods that return it.
 - Ability to create a Task which is initially finished (is this useful enough to be worthwhile?)

With those, or at least rescheduling, we could deprecate Task and friends.


One minor annoyance - worth dealing with? - is that the argument to Task.schedule() is an int;  however both ExecutorService and the underlying java.util.Timer accept long.  I'm guessing this is just an old accident.  All tests pass with an overloaded version of Task.schedule() that takes a long (not in the current patch).  If we do not decide to deprecate Task, should we deprecate schedule(int)? Or just not worry about it?  Or make the overload public?

Re {@inheritDoc} - when I build javadoc for openide.util, I do not get any inherited javadoc embedded (presumably because it would have to be downloaded).

Will this actually work for production builds of the javadoc?

(In reply to comment #11)
> Re {@inheritDoc} - when I build javadoc for openide.util, I do not get any
> inherited javadoc embedded (presumably because it would have to be downloaded).

Might be limited when -linkoffline is used. Would not be relevant for editor code completion. Don't worry about it, use it anyway. People can always click on the super method anyway. It is more important to avoid redundancy.

Created attachment 94350 [details]
Patch which also provides re-schedulable transactions

This patch adds the following, with tests:

public interface ReschedulableFuture<T> extends ScheduledFuture<T>{
    ScheduledFuture<T> schedule (long delay, TimeUnit unit);
    ScheduledFuture<T> schedule (long delayMillis);
}

and

    public ReschedulableFuture<?> createReschedulable(Runnable runnable, boolean initiallyFinished);
    public ReschedulableFuture<?> createReschedulable(Runnable runnable);

which fulfill the same role as creating and rescheduling an RP.Task within the conceptual space of java.util.concurrent.

One thing to think about:  ReschedulableFuture.schedule(*) could have a void return type;  however, there would not be any way to compatibly change it later if we decide to support reschedulable Callables, where there is a legitimate for monitoring and getting a result from an individual run.

Y02 I do not think we shall introduce our own ReschedulableFuture. The functionality is available with RP.Task and we don't need yet another way to do the same thing. If you feel strongly that it is needed, please donate that interface to java.util.concurent

Btw. if you need to reproduce race condition in test, use logging control

Integrated, minus ReschedulableFuture, in main/ 7e4413ade2a2

(In reply to comment #15)
> in main 7e4413ade2a2

In the future please only push changes to core modules to core-main, as this ensures that the changes are properly vetted by the long list of tests that NB-Core-Build runs. (If you do not have a clone of core-main handy to push to, send a bundle or patch to someone who does.)

Tim, 7e4413ade2a2 is about "Updating Paint Application sample for 6.9" :-)

Integrated into 'main-golden', will be available in build *201002261426* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/99c6bde5f215
User: Tim Boudreau <tboudreau@netbeans.org>
Log: #180386 - support for Callable and Future in RequestProcessor