----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55742/#review162862 -----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java (line 1583) <https://reviews.apache.org/r/55742/#comment234224> Won't flipping the interrupt bit cause problems with sending profile messages below? geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AcceptorImplObserver.java (line 23) <https://reviews.apache.org/r/55742/#comment234223> A few thoughts: 1) The singleton instance is not thread-safe. You need to protect both reads and writes. 2) I'd prefer to see AcceptorImplObserver be an interface instead of an abstract class. Seems like the instance member more logically belongs to the AcceptorImpl class instead of delegating to a singleton. - Anthony Baker 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 > >
