bbeaudreault opened a new pull request, #5154:
URL: https://github.com/apache/hbase/pull/5154
The basic idea here is we should usually have two threads: the main thread
and a reader thread. Writes come through the main thread, which also handles
calling setupIOStreams if the connection is not yet made. The reader thread is
continually polling for work, calling `readResponse()` when calls are found.
The writer methods are all synchronized, while the reader thread is not. The
reader thread uses a `waitForWork()` poll method, which is itself synchronized.
If an exception occurs while writing a request, `closeConn` will be called.
This interrupts and nulls out the reader `thread`, along with the socket and
streams, and fails all calls that were pending read. The next write to come in
will go through `setupIOStreams()`, which will create new sockets/streams and
start a new reader thread.
In an ideal world, when `closeConn` is called, the reader thread will be
waiting on the `wait()` call in `waitForWork()`. In that case, it's likely (not
guaranteed) that when the thread is interrupted by `closeConn` the `wait()`
will finish and the first check in `waitForWork()` will be true (`thread ==
null`). In that case, the reader thread will properly end.
Synchronization order is unspecified. So it's possible that while the
existing `writeRequest`/`closeConn` was running, another write came in and was
waiting on the monitor. When the original call releases the monitor, the new
write comes in and since the socket is null, goes through `setupIOStreams()`.
In this case, when the `wait()` finishes in `waitForWork()` it will check for
`thread == null` and the thread _will not_ be null. It will have changed to a
new thread, not the current thread.
This can also occur if closeConn is called while we are in `readResponse()`,
which is not synchronized at all. The same scenario can happen where a new
write can come in after `closeConn` which creates a new thread before
readResponse finishes. So it'll go into waitForWork() and see that thread is
not null, and then the old reader thread never dies.
----
A larger refactor is probably in order here, if BlockingRpcConnection
weren't being replaced. As it is, I solved this issue by adding two things:
1. Add a check for isCurrentThreadExpected, which checks if
`Thread.currentThread()` is equal to `thread`. I added this check in three
places where better handling is necessary:
1. In the waitForWork loop, if thread != null. We should check that
thread is also the current thread, otherwise we need to exit.
1. When handling InterruptedException. In this case we want to call
closeConn if the interrupt itself didn't come from closeConn. For example, if
the process is ending or an external actor interrupted us.
1. In readResponse, when deciding whether to closeConn when an error
occurs. I've seen some cases where we end up unnecessarily closing and
restarting the same connection thread multiple times because closeConn causes
readResponse to fail, but in the meantime a new connection thread was created.
The readResponse failure calls closeConn again even though the new connection
is ok.
1. Synchronize the reader threads, so that two reader threads can't read
from the same socket. This causes corruption and other oddities.
----
I don't really see any specific tests for BlockingRpcConnection, beyond
TestBlockingIPC. I don't really know how I'd add a new test for this logic
given our setup. Currently I'm working on doing some manual testing of this
change in our environment with a live cluster and lots of multigets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
