----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55742/#review162978 -----------------------------------------------------------
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java (line 87) <https://reviews.apache.org/r/55742/#comment234358> You could use ThreadUtils.dumpMyThreads() here geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java (line 128) <https://reviews.apache.org/r/55742/#comment234362> Is this a sufficient check to determine that the problem has been fixed? isRunning() looks at the variable "shutdown", which is set to true at the top of the close() method. I think this code needs to check & ensure that both of the AcceptorImpl thread pools have been shut down. geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java (line 160) <https://reviews.apache.org/r/55742/#comment234360> If the test fails this code won't be executed. Move it to an @After - Bruce Schuchardt On Jan. 24, 2017, 11:31 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, 11:31 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 > >
