Fix over acquiring of tasks in MultiThreadAgent #496
Conversation
frsyuki
force-pushed
the
max-threads-overacquire-fix
branch
from
a996234 to
d3950b4
Compare
| @@ -98,12 +118,18 @@ public void run() | |||
| logger.error("Uncaught exception. Task queue will detect this failure and this task will be retried later.", t); | |||
| errorReporter.reportUncaughtError(t); | |||
| } | |||
| finally { | |||
| synchronized (taskCountLock) { | |||
| activeTaskCount--; | |||
There was a problem hiding this comment.
Using AtomicInteger makes it a bit simpler.
There was a problem hiding this comment.
With SynchronousQueue, calling executor.submit() can be blocked holding taskCountLock. As a result, a thread that tries to decrement the counter here can be potentially blocked forever?
There was a problem hiding this comment.
SynchronousQueue is used with maximumPoolSize: Integer.MAX_VALUE. So it's usually okay. But it seems a bit naive to me...
There was a problem hiding this comment.
That's a good point 👍 I changed the code to use AtomicInteger. Now still AgentOverAcquireIT passes.
|
LGTM overall 👍 But there is a room to improve around https://github.com/treasure-data/digdag/pull/496/files#r104843071 to reduce a potential deadlock risk. Can you take a look at it? |
MultiThreadAgent.run needs to know how many tasks can start immediately. But it doesn't have to be exact number. It is OK to acquire smaller number as long as it can start the tasks immediately.
|
LGTM 👍 > 6926ec0 |
Fixes #487.