> On Jan. 20, 2017, 10:11 p.m., Bruce Schuchardt wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java, > > line 1582 > > <https://reviews.apache.org/r/55742/diff/2/?file=1610853#file1610853line1582> > > > > Typically we set the interrupt bit on the thread at the end of the > > block of code where we ignore interrupts. You can see examples of this in > > ShutdownAllRequest, SearchLoadAndWriteProcessor and other places (look for > > Thread.currentThread().interrupt()). > > > > Is there a reason we aren't doing that here? > > Galen O'Sullivan wrote: > No, no particular reason. I'm not doing it because the code that was here > before didn't do it. I do see the interrupted flag used elsewhere, but not > consistently. Do we use it just to signal that we should clean up as well as > possible and shut down or for some reason as well? > > For example, `ShutdownAllRequest.send()` catches `InterruptedRequest` and > sets the interrupted flag, but then it immediately tries a `Thread.sleep()` > and discards any `InterruptedRequest` that throws: > ``` > if (interrupted) { > Thread.currentThread().interrupt(); > } > > try { > Thread.sleep(3 * SLEEP_TIME_BEFORE_DISCONNECT_DS); > } catch (InterruptedException e) { > } > ```
I've opened a ticket for `ShutdownAllRequest.send`: https://issues.apache.org/jira/browse/GEODE-2348 - Galen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55742/#review162512 ----------------------------------------------------------- On Jan. 24, 2017, 10 p.m., Galen O'Sullivan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55742/ > ----------------------------------------------------------- > > (Updated Jan. 24, 2017, 10 p.m.) > > > Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo > Kohlmeyer. > > > Bugs: GEODE-2324 > https://issues.apache.org/jira/browse/GEODE-2324 > > > Repository: geode > > > Description > ------- > > [GEODE-2324] fix AcceptorImpl cleanup. > > * Catch InterruptedException so cleanup continues. > * Remove top-level exception handler to avoid similar mistakes in the future. > * Fix a synchronization bug that could cause AcceptorImpl to try to shut down > twice. > * Fix what looks like a bug where if closing the socket throws and > IOException, we fail to shut anything else down, though we still have > ourselves marked as shut down. > > I'm a little skeptical of whether we're waiting at all for the queue to shut > down, as the thread could have been marked as interrupted for quite a while > before and never noticed it until the thread got parked. This may warrant > more investigation. > > > Diffs > ----- > > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java > 060683de6 > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AcceptorImplObserver.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java > PRE-CREATION > > geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplJUnitTest.java > 7aa11b7ca > > Diff: https://reviews.apache.org/r/55742/diff/ > > > Testing > ------- > > precheckin passed on (3), will run on (5). > > > Thanks, > > Galen O'Sullivan > >
