Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ScheduledExecutorServiceDelegate violates contract of ScheduledExecutorService #7611

Closed
jerrinot opened this issue Feb 25, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@jerrinot
Copy link
Contributor

commented Feb 25, 2016

Contract of scheduleAtFixedRate says:

     * If any execution of this task
     * takes longer than its period, then subsequent executions
     * may start late, but **will not concurrently execute**.

However the ScheduledExecutorServiceDelegate wraps tasks in ScheduledTaskRunner which delegates to a different executor. As a consequence a task can be executed concurrently thus a contract of ScheduledExecutorService is violated.

This leads to surprises. this was seen in a testing environment:

Exception in thread "cached4" java.lang.IllegalArgumentException: Comparison method violates its general contract!
    at java.util.TimSort.mergeHi(TimSort.java:895)
    at java.util.TimSort.mergeAt(TimSort.java:512)
    at java.util.TimSort.mergeCollapse(TimSort.java:437)
    at java.util.TimSort.sort(TimSort.java:241)
    at java.util.Arrays.sort(Arrays.java:1512)
    at java.util.ArrayList.sort(ArrayList.java:1454)
    at java.util.Collections.sort(Collections.java:175)
    at com.hazelcast.map.impl.eviction.ExpirationManager$ClearExpiredRecordsTask.sortPartitionContainers(ExpirationManager.java:127)
    at com.hazelcast.map.impl.eviction.ExpirationManager$ClearExpiredRecordsTask.run(ExpirationManager.java:121)
    at com.hazelcast.util.executor.CachedExecutorServiceDelegate$Worker.run(CachedExecutorServiceDelegate.java:212)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)
    at com.hazelcast.util.executor.HazelcastManagedThread.executeRun(HazelcastManagedThread.java:76)
    at com.hazelcast.util.executor.HazelcastManagedThread.run(HazelcastManagedThread.java:92)

@jerrinot jerrinot added this to the 3.7 milestone Feb 25, 2016

@mtopolnik

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2016

The way I see this, ScheduledExecutorServiceDelegate is-not-a ScheduledExecutorService, so it should stop implementing that interface. We may define a new interface which captures the semantics we want.

@jerrinot

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2016

we can change either contract or behavior.
contract change is less risky, but it can still surprise us in a future. having a guarantee a task is never executed concurrently is useful.

@jerrinot jerrinot changed the title ScheduledExecutorServiceDelegate violates contracts of ScheduledExecutorService ScheduledExecutorServiceDelegate violates contract of ScheduledExecutorService Feb 26, 2016

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 7, 2016

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed the SchedulingExecutorServiceDelegate to avoid confusion.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 8, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

@jerrinot jerrinot self-assigned this Mar 8, 2016

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 9, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 9, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor

jerrinot added a commit to jerrinot/hazelcast that referenced this issue Mar 9, 2016

SchedulingExecutorServiceDelegate does not implement j.u.c.ScheduledE…
…xecutorService

Fixes hazelcast#7611

Our scheduler has a different contract hence it should not
implement the interface from JDK.

I renamed it to SchedulingExecutorServiceDelegate to avoid confusion
with scheduler from JDK.
Also I changed the method name from scheduleAtFixedRate to scheduleWithRepetition
to emphasize it has a different semantic.

The scheduleWithFixedDelay was removed as it never worked and it
effectively behaved as the scheduleAtFixedRate anyway.

The same changes are done in ClientExecutionService and NearCacheExecutor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.