1.x: fix ExecutorScheduler and GenericScheduledExecutorService reorder bug

[akarnokd]

This PR relates to the failure of errorThrownIssue1685.

The underlying problem was with the GenericScheduledExecutorService. By being multi-threaded, tasks scheduled from the same thread one after the other may get reordered because different worker threads inside the pool could pick them up at the same time. In this case, there is no guarantee they keep their FIFO order.

(I currently have no idea how one can use trampolining for this case; subsequent tasks may have any relative delays in respect to each other.)

The solution creates N single threaded ScheduledExecutorServices and getInstance() hands one of them out. In turn ExecutorService takes one in its worker upfront.

However, there is still the problem when the programmer uses a multi-threaded ScheduledExecutorService with Schedulers.from() when the same issue comes back. A solution to that problem would be to always use the new GenericScheduledExecutorService for delaying timed tasks.

[artem-zinnatullin]

Ew

I definitely don't like that library will create executor when this class will be loaded. Even without threads used it looks strange.

Maybe return new each time getInstance()?

[akarnokd]

Executors don't start their thread until the first task is submitted.

[stevegury]

I have trouble understanding how this solve the problem, could you please elaborate?
The only thing I see that we could do to force the FIFO order is to cap the thread number to 1, right?

[akarnokd]

Yes, ExecutorScheduler's worker needs a helper ScheduledExecutorService with a single thread only. But we don't want all ExecutorSchedulers to wait in a single thread for their time to run. This change, similar to how computation scheduler works, hands out single hreaded ScheduledExecutorServices on demand.

[zsxwing]

Nice catch.

However, there is still the problem when the programmer uses a multi-threaded ScheduledExecutorService with Schedulers.from() when the same issue comes back. A solution to that problem would be to always use the new GenericScheduledExecutorService for delaying timed tasks.

I vote for always use the new GenericScheduledExecutorService for delaying timed tasks.. Correctness is more important than performance. Moreover, when people use schedule(action, time), they usually don't want the codes to run as fast as possible, so the performance lost doesn't really matter.

[stevegury]

Agree with @zsxwing that correctness is more important than performance.

[zsxwing]

count should be replaced with 1 to guarantee FIFO. Right?

[akarnokd]

Right, I'll fix them shortly.

[akarnokd]

Updated.

[zsxwing]

👍

[stevegury]

👍