[REM3-282] Update the main connection attempt notifier to resolve all pending connection attempts #116
Conversation
|
It looks like this works because it will spin until the state is no longer MaybeShared. Is that correct? |
|
Yes. It seems this is necessary to ensure the notifier added in None#getConnection gets run first once the original connection is established. |
|
I assume then given that the remaining code in MaybeShared#getConnection() is unchanged that it is working correctly, i.e. the first time the IOFuture is returned, it completes successfully. Is that consistent with your observations? |
|
Yes, that is consistent with what I'm seeing, the remaining code is working properly. |
|
OK. We definitely can't introduce a spin loop at this point (because then only the first (well, first and second really) attempt to connect will be asynchronous). So that raises the question: why does it fail? Theoretically, it should be possible to return the same IOFuture instance for all attempts with a given identity and they should all function identically. Once the connection completes, all the waiting threads would get unblocked at the same time. If a thread is stuck in ioFuture.get(), or there is a notifier which is never being called, this would point to a fundamental bug in AbstractIoFuture. |
|
OK. We are ending up with a thread stuck in AbstractIoFuture.get() (see http://pastebin.test.redhat.com/489203). Just heading out to a doctor's appointment. Will add more details here when I get back if I can or tomorrow morning. |
|
OK. I'll see if I can find anything obviously wrong with AbstractIoFuture. |
|
OK the notifier thing is making sense to me. Essentially it splices the pending-attempt future to itself when the notifiers run in the wrong order (which they're usually going to do), which is pointless and prevents the connection from ever resolving. It looks like what is needed is a way for the primary notifier to resolve all the pending attempts, rather than creating secondary notifiers for each one. |
|
Any thoughts on how to go about doing that? |
|
The main notifier has a ref to the Maybe state. It could iterate the map and splice each future. Some care would have to be taken to avoid a race condition. |
|
Thanks, David! I'll try that. |
|
I've updated the main notifier to resolve all pending attempts. |
fjuma
changed the title
| if (futureResult != null) { | ||
| return futureResult.getIoFuture(); | ||
| try { | ||
| pendingAttemptsLock.readLock().lock(); |
There was a problem hiding this comment.
Two things to note:
- Lock acquisition should always happen before the
try, otherwise if the lock fails for whatever reason, the unlock is called when the thread doesn't own the lock, causing an IllegalMonitorStateException or similar. - ReentrantReadWriteLock is too heavy for this usage since the inside of the block is short-lived and non-blocking. It's probably better to just drop to a plain map and make it synchronized, and we can revisit later if there is substantial contention.
… pending connection attempts
|
Fixed. |
See https://issues.jboss.org/browse/REM3-282 for details on this one.
WDYT?