GitHub user eshioji opened a pull request:
https://github.com/apache/storm/pull/566
[STORM-839] Deadlock hazard in backtype.storm.messaging.netty.Client
(I accidentally did a PR against master with the same content, please
ignore that one)
This fixes the reported deadlock between `disruptor-worker-transfer-queue`
thread and `client-worker` thread, which seem to have been introduced by
STORM-329.
After reviewing the `v0.9.4` code, my conclusion was that the background
flushing task can be removed without real change in the behavior. By doing
this, the synchronization that was involved in the deadlock can be removed
altogether.
My reasoning is as follows:
- One has three option to deal with Netty's buffer filling up:
1. Discard incoming new messages
2. Block client thread until there is space (back pressure)
3. Keep buffering up until OOME is thrown
- My guess is that the `v0.9.4` code attempted to implement option (i),
but actually the behavior is option (iii). When Netty's `Channel.isWritable`
returns false in `Client.send`, the thread avoids writing to the `Channel` and
leaves messages in the `Channel.messageBatch` field, which the background task
flushes when the `Channel.isWritable` starts to return true again. However, if
`Client.send` is called again before this (which should be common), the content
of `Channel.messageBatch` is written and buffered on the `Channel` anyways,
because `flushMessages` does not check `Channel.isWritable`.
- AFAIU we like option (ii), but this requires more work. Between option
(i) and option (iii), IMO (iii) is superior because if OOME happens, the user
can reduce MAX_PENDING_TUPLES to prevent this. Discarding messages would be
harder to diagnose.
- If we are content with option (ii), we can remove the background
flushing task and related synchronization altogether, removing the source of
deadlock. We'd simply write and buffer onto the unbounded buffer of `Channel`,
and if there are too many pending messages, the worker will die of OOME, and
the user should reduce this with indirect means like reducing
MAX_PENDING_TUPLES until option (ii) is implemented in the future.
Thoughts @miguno ?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/eshioji/storm STORM-839
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/566.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #566
----
commit ed8ab3ec194f19c75fc2f5c000609204f04b50e8
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-28T19:42:05Z
Simplified the flow and removed the lock that was causing the deadlock
commit 91b8eb3840432e47b79f40abebec8304627732a8
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-28T19:46:17Z
Bump version
commit b7d84bdc7fd3de34f45a94131cdbb6bfbd3763dc
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-28T21:27:31Z
Remove background flushing because it doesn't seem necessary. Netty's
Channel queues up written data on an unbounded buffer. The background flushing
seems to have been added to avoid this, but in practice it was probably doing
it anyways because flushMessages(), which is called by send() doesn't check for
isWritable. Moreover, queuing on an unbounded buffer seems fine because back
pressure is provided by MAX_PENDING_TUPLE. If OOME occurs due to this buffer
overflowing, it seems reasonable that one has to reduce MAX_PENDING_TUPLE,
rather than Storm trying to cope with it by dropping messages.
commit 679e42bc1e38f51c2759667b03cb45322c6a793b
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-28T21:31:35Z
Change to a SNAPSHOT version for deployment purposes
commit 27a92e2aa3488c0203f500306e0583ff9e7e1e82
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-29T09:32:16Z
Remove (now) dead comment and code
commit 09bf6e1b5d9d351f2a60cd9a32e0239752cf437a
Author: Enno Shioji <eshi...@gmail.com>
Date: 2015-05-29T10:23:46Z
Merge branch '0.9.x-branch' into STORM-839
Conflicts:
examples/storm-starter/pom.xml
external/storm-hbase/pom.xml
external/storm-hdfs/pom.xml
external/storm-kafka/pom.xml
pom.xml
storm-buildtools/maven-shade-clojure-transformer/pom.xml
storm-core/pom.xml
storm-dist/binary/pom.xml
storm-dist/source/pom.xml
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
