Fixing executor and flow control issues in subscriber #1915

davidtorres · 2017-04-13T20:24:44Z

The current subscriber logic has a few concurrency issues explained as follows:

The flow controller blocks the user's executor threads; so in the event of all the threads getting blocked because of flow control limits, the subscriber won't be able to dispatch messages to the user code hence unable to release flow control permits and to make forward progress; a deadlock. This change makes the flow control non-blocking and instead regulates the flow of messages by either suspending the streaming or not issuing more pulls but until permits are released from flow controller
Lease extensions are handled under the user's executor, in the event of the user code monopolizing all the threads in the executor, the subscriber won't be able to execute any lease extension which will make the leases to expire and increase the chances of message redelivery; if there is a time when it is more important to maintain lease extensions is when the user code is busy working on the messages. This code change sets up a separate small (two threads) shared-across-subscribers executor for taking care of lease extensions, under my load tests (with >600 hundred subscriber objects) this executor performed well not showing signs of work backup, this is mainly because the work of scheduling and sending lease extensions is fairly cheap.
Flow control permits were being drawn on a per batch basis while the messages were dispatch one at a time, this could cause a deadlock if ever receiving a batch larger than the limit of message in the flow controller. This change makes the code reserve flow control permits on a per messages basis as they are dispatched to the user code.

Lastly, I have also address a minor issues which is the inability of choosing the number of messages to pull per pull request, now such number is taken from the flow controller settings.

This change fixes #1868, fixes #1865 and fixes #1855

user code - Separated the handling of batched received messages from the per-message dispatching of message to the user code. In order to accomplish this I had to keep the batch in memory while the processing of each message will draw from the in-memory batch until completely depleted. - Drawing flow controller permits on a per message basis (used to try to draw permits for the whole batch potentially deadlocking the whole subscriber), this addresses the deadlock condition raised in googleapis#1868 - No longer using blocking flow controller, instead pausing and resuming pulls/streamed-messages based on the flow controller feedback and when new permits become available. - A separate executor for alarms (2 threads in it has showed up to scale pretty well with many subscriber, given our ack operations are pretty lightweight) - Setting the maximum of messages to pull per request based on the number requested by the user in the flow controller (if any), this in a best effort addresses googleapis#1868 Fixes googleapis#1868, fixes googleapis#1865 and fixes googleapis#1855

davidtorres · 2017-04-13T20:25:18Z

@pongad @garrettjonesgoogle all yours to review thanks

garrettjonesgoogle · 2017-04-14T14:16:29Z

I will have @pongad review it first.

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/spi/v1/MessageDispatcher.java

+      return messages;
+    }
+
+    public void done() {


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/spi/v1/MessageDispatcher.java

@@ -57,6 +61,9 @@
  @VisibleForTesting static final Duration PENDING_ACKS_SEND_DELAY = Duration.millis(100);
  private static final int MAX_ACK_DEADLINE_EXTENSION_SECS = 10 * 60; // 10m

+  private static final ScheduledExecutorService alarmsExecutor =


And some other minor changes addressing feedback

garrettjonesgoogle · 2017-04-20T11:23:46Z

Looks like there is some fallout from the merge that is causing the build to fail

google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/spi/v1/Subscriber.java

+            builder
+                .flowControlSettings
+                .toBuilder()
+                .setLimitExceededBehavior(LimitExceededBehavior.ThrowException)


google-cloud-pubsub/src/main/java/com/google/cloud/pubsub/spi/v1/MessageDispatcher.java

+    }
+  }
+
+  public void processReceivedMessages(List<ReceivedMessage> messages, Runnable doneCallback) {


by the user, this allows for proper unit testing plus adds the ability to the user to override the executor.

garrettjonesgoogle · 2017-04-21T03:01:46Z

LGTM

coveralls · 2017-04-21T03:02:36Z

Coverage decreased (-0.01%) to 80.886% when pulling f716280 on davidtorres:master into a722e95 on GoogleCloudPlatform:master.

davidtorres added 2 commits April 13, 2017 16:00

Fix codacy complains

ef1b25a

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 13, 2017

davidtorres added 2 commits April 14, 2017 09:46

Merge branch 'master' into master

361b920

Merge branch 'master' into master

e3ac433

pongad reviewed Apr 17, 2017

View reviewed changes

davidtorres added 3 commits April 19, 2017 22:20

Making the executor use deamon threads

7f0cdfd

And some other minor changes addressing feedback

Merge branch 'master' into master

bb1bf4f

Merge branch 'master' of github.com:davidtorres/google-cloud-java

eec304e

garrettjonesgoogle reviewed Apr 20, 2017

View reviewed changes

Fixing merging issues as well as making the alarms executor configurable

f716280

by the user, this allows for proper unit testing plus adds the ability to the user to override the executor.

garrettjonesgoogle merged commit f1d4d87 into googleapis:master Apr 21, 2017

This was referenced May 1, 2017

FlowController.reserve - deadlock on acquireUninterruptibly() googleapis/gax-java#268

Closed

pubsub Limit the number of subscriber threads how? #1827

Closed

saturnism mentioned this pull request May 9, 2017

Pub/Sub Subscriber Deadlock w/ Executor pool size of 1 #2041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixing executor and flow control issues in subscriber #1915

Fixing executor and flow control issues in subscriber #1915

Uh oh!

davidtorres commented Apr 13, 2017

Uh oh!

davidtorres commented Apr 13, 2017

Uh oh!

garrettjonesgoogle commented Apr 14, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Apr 20, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Apr 21, 2017

Uh oh!

coveralls commented Apr 21, 2017

Uh oh!

Uh oh!

Fixing executor and flow control issues in subscriber #1915

Fixing executor and flow control issues in subscriber #1915

Uh oh!

Conversation

davidtorres commented Apr 13, 2017

Uh oh!

davidtorres commented Apr 13, 2017

Uh oh!

garrettjonesgoogle commented Apr 14, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Apr 20, 2017

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

garrettjonesgoogle commented Apr 21, 2017

Uh oh!

coveralls commented Apr 21, 2017

Uh oh!

Uh oh!