ConcurrentSubscripiton avoid concurrent access for invalid demand #1015

Scottmitch · 2020-04-14T20:48:57Z

Motivation:
ConcurrentSubscription currently propagates invalid demand without any
concurrency protection. In general this is invalid use of the API but
may invalidate underlying data structures that are not thread safe and
result in undefined results.

Modifications:

Use a simpler locking scheme inspired by Publisher#flatMapMerge design
which allows for re-entry and also notification is another thread has
attempted to acquire the lock which will trigger re-processing.

Result:
ConcurrentSubscripiton no longer allows any concurrent access and uses a
more common/shareable locking utility.

...lk-concurrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentUtils.java

normanmaurer · 2020-04-15T06:12:51Z

...urrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentSubscription.java

+    private static long mapInvalidRequestN(long n) {
+        // We map zero to a negative number because zero could later be overwritten by a subsequent legit value of
+        // n, and we want to ensure the invalid use gets propagated.
+        return n == CANCELLED ? CANCELLED + 1 : n == 0 ? -1 : n;


nit: Imho chaining stuff like this is really not readable. Consider using a branch

@Scottmitch ^^

I prefer to keep the code as is as it is more compact and I don't think readability is dramatically different between the two options:

current option:

return n == CANCELLED ? CANCELLED + 1 : n == 0 ? -1 : n;

suggested option:

if (n == CANCELLED) { return CANCELLED + 1; } return n == 0 ? -1 : n;

...urrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentSubscription.java

...k-concurrent-api/src/test/java/io/servicetalk/concurrent/api/PublisherFlatMapSingleTest.java

Scottmitch · 2020-04-28T04:46:43Z

humm ... ci failures

Creating servicetalk-java8-prb_runtime-setup_1 ... done
Error: No such container: f7df93647ccd8b7580a740e917771dbc1589b9f79d8694758bcf73a244e776d8
21:45:17 No such container: f7df93647ccd8b7580a740e917771dbc1589b9f79d8694758bcf73a244e776d8
21:45:17 Build step 'Execute shell' marked build as failure

Scottmitch · 2020-04-28T04:46:53Z

@servicetalk-bot - test this please

Scottmitch · 2020-04-28T15:19:10Z

build failure attributed to #744

Scottmitch · 2020-04-28T15:19:23Z

@servicetalk-bot - test this please

Scottmitch · 2020-04-28T16:27:18Z

ci failure...

08:28:08 
Creating servicetalk-java8-prb_runtime-setup_1 ... done
time="2020-04-28T15:28:09Z" level=error msg="error waiting for container: context canceled"
08:28:09 Error response from daemon: failed to get network during CreateEndpoint: network 82cd76a80b00834668362278f1efff2e358505fff034522ffee45181c158dfe9 not found
08:28:09 Build step 'Execute shell' marked build as failure

Scottmitch · 2020-04-28T16:27:24Z

@servicetalk-bot - test this please

NiteshKant

Nice simplification of ConcurrentSubscription 💯

...lk-concurrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentUtils.java

...urrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentSubscription.java

Scottmitch · 2020-04-30T21:31:41Z

suspected CI related failure ... https://ci.servicetalk.io/job/servicetalk-java11-prb/1280/consoleText

BUILD SUCCESSFUL in 9m 56s
301 actionable tasks: 242 executed, 59 up-to-date
removal of container ad39f54ec9fb47d45e167e146414ef1ca48efa7f3bc34ca711a307b6f548bfa3 is already in progress
Build step 'Execute shell' marked build as failure

Scottmitch · 2020-04-30T21:32:05Z

@servicetalk-bot - test this please

Scottmitch · 2020-04-30T22:02:48Z

Benchmarks were revised to provide multi-threaded perf for non-reentrant case and more stack depth options for recursion.

Summary:

`tryAcquireReentrantLock`

gains against master for reentrant (+151% to +500% depending upon recursion depth)
gains against tryAcquireLock for reentrant (bigger margins than master)
gains against master for single threaded non-reentrant (+28%)
similar to master for multi threaded non-reentrant
similar to tryAcquireLock for non-reentrant

`tryAcquireLock`

regresses against master for reentrant (-10% to -30% depending upon recursion depth)

## master
Benchmark                                              (stackDepth)   Mode  Cnt         Score        Error  Units
ConcurrentSubscriptionReentrantBenchmark.singleThread             2  thrpt    5  24318761.311 ± 965534.969  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread            10  thrpt    5   4608999.521 ± 475365.160  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread           100  thrpt    5    453085.063 ±  53186.602  ops/s

Benchmark                                      Mode  Cnt         Score        Error  Units
ConcurrentSubscriptionBenchmark.multiThread   thrpt    5   6753766.730 ± 477061.905  ops/s
ConcurrentSubscriptionBenchmark.singleThread  thrpt    5  58855485.836 ± 715964.377  ops/s

## Reentrant (using tryAcquireReentrantLock)
Benchmark                                              (stackDepth)   Mode  Cnt         Score        Error  Units
ConcurrentSubscriptionReentrantBenchmark.singleThread             2  thrpt    5  61086581.693 ± 866190.998  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread            10  thrpt    5  27976426.295 ± 842739.171  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread           100  thrpt    5   2248121.486 ±  54539.770  ops/s

Benchmark                                      Mode  Cnt         Score         Error  Units
ConcurrentSubscriptionBenchmark.multiThread   thrpt    5   6491110.907 ±  317372.027  ops/s
ConcurrentSubscriptionBenchmark.singleThread  thrpt    5  76085499.345 ± 1123039.174  ops/s

## Non-reentrant (using tryAcquireLock)
Benchmark                                              (stackDepth)   Mode  Cnt         Score         Error  Units
ConcurrentSubscriptionReentrantBenchmark.singleThread             2  thrpt    5  21694566.056 ± 2973401.571  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread            10  thrpt    5   3410420.825 ±  384962.783  ops/s
ConcurrentSubscriptionReentrantBenchmark.singleThread           100  thrpt    5    323562.643 ±   37266.246  ops/s

Benchmark                                      Mode  Cnt         Score         Error  Units
ConcurrentSubscriptionBenchmark.multiThread   thrpt    5   6423213.603 ±  601263.682  ops/s
ConcurrentSubscriptionBenchmark.singleThread  thrpt    5  75500536.822 ± 5127380.129  ops/s

NiteshKant

Final comments then LGTM

servicetalk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/ConcurrentUtilsTest.java

...lk-concurrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentUtils.java

...urrent-internal/src/main/java/io/servicetalk/concurrent/internal/ConcurrentSubscription.java

Scottmitch · 2020-05-04T20:25:38Z

build failure attributed to #1040

Motivation: ConcurrentSubscription currently propagates invalid demand without any concurrency protection. In general this is invalid use of the API but may invalidate underlying data structures that are not thread safe and result in undefined results. Modifications: - Use a simpler locking scheme inspired by Publisher#flatMapMerge design which allows for re-entry and also notification is another thread has attempted to acquire the lock which will trigger re-processing. Result: ConcurrentSubscripiton no longer allows any concurrent access and uses a more common/shareable locking utility.

Scottmitch · 2020-05-05T01:24:35Z

discussed relaxing the exception handling with @NiteshKant offline. I've modified this PR to incorporate @NiteshKant's suggestions and we can enhance later if necessary.

NiteshKant

💯

…ple#1015) Motivation: ConcurrentSubscription currently propagates invalid demand without any concurrency protection. In general this is invalid use of the API but may invalidate underlying data structures that are not thread safe and result in undefined results. Modifications: - Use a simpler locking scheme inspired by Publisher#flatMapMerge design which allows for re-entry and also notification is another thread has attempted to acquire the lock which will trigger re-processing. Result: ConcurrentSubscripiton no longer allows any concurrent access and uses a more common/shareable locking utility.

Scottmitch requested review from NiteshKant and idelpivnitskiy April 14, 2020 20:48

normanmaurer requested changes Apr 15, 2020

View reviewed changes

Scottmitch force-pushed the concurrent_subscription branch 2 times, most recently from 825e5c2 to 71b94a4 Compare April 28, 2020 04:37

Scottmitch force-pushed the concurrent_subscription branch from 71b94a4 to 064f892 Compare April 28, 2020 04:55

Scottmitch mentioned this pull request Apr 28, 2020

JerseyDataJacksonTestSuite > ExecutionStrategyTest.testResource test failure #744

Closed

NiteshKant reviewed Apr 29, 2020

View reviewed changes

Scottmitch requested review from normanmaurer and NiteshKant April 29, 2020 22:32

NiteshKant reviewed May 4, 2020

View reviewed changes

Scottmitch mentioned this pull request May 4, 2020

JerseyDataJacksonTestSuite > SynchronousResourceTest.postTextResponse #1040

Closed

Scottmitch force-pushed the concurrent_subscription branch from 02f1224 to d259d2c Compare May 4, 2020 21:02

Scottmitch added 7 commits May 4, 2020 15:50

review feedback, tests, benchmark

814dfa7

add fast path for no concurrency case

a372625

add more tests

52b7130

more tests, split benchmarks

71165d8

fix comment

39cbe87

add more tests

14ab4d2

review comments

5c9f925

Scottmitch force-pushed the concurrent_subscription branch from d259d2c to 5c9f925 Compare May 4, 2020 22:53

relax locking release strategy and exception catching

daf1ab7

NiteshKant approved these changes May 5, 2020

View reviewed changes

Scottmitch merged commit e57a01c into apple:master May 5, 2020

Scottmitch deleted the concurrent_subscription branch May 5, 2020 01:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ConcurrentSubscripiton avoid concurrent access for invalid demand #1015

ConcurrentSubscripiton avoid concurrent access for invalid demand #1015

Scottmitch commented Apr 14, 2020

normanmaurer Apr 15, 2020

normanmaurer Apr 28, 2020

Scottmitch Apr 29, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

NiteshKant left a comment

Scottmitch commented Apr 30, 2020

Scottmitch commented Apr 30, 2020

Scottmitch commented Apr 30, 2020 •

edited

NiteshKant left a comment

Scottmitch commented May 4, 2020

Scottmitch commented May 5, 2020

NiteshKant left a comment

ConcurrentSubscripiton avoid concurrent access for invalid demand #1015

ConcurrentSubscripiton avoid concurrent access for invalid demand #1015

Conversation

Scottmitch commented Apr 14, 2020

normanmaurer Apr 15, 2020

Choose a reason for hiding this comment

normanmaurer Apr 28, 2020

Choose a reason for hiding this comment

Scottmitch Apr 29, 2020

Choose a reason for hiding this comment

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

Scottmitch commented Apr 28, 2020

NiteshKant left a comment

Choose a reason for hiding this comment

Scottmitch commented Apr 30, 2020

Scottmitch commented Apr 30, 2020

Scottmitch commented Apr 30, 2020 • edited

tryAcquireReentrantLock

tryAcquireLock

NiteshKant left a comment

Choose a reason for hiding this comment

Scottmitch commented May 4, 2020

Scottmitch commented May 5, 2020

NiteshKant left a comment

Choose a reason for hiding this comment

Scottmitch commented Apr 30, 2020 •

edited

`tryAcquireReentrantLock`

`tryAcquireLock`