> On Jan. 24, 2017, 10:32 p.m., Anthony Baker wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java, > > line 1588 > > <https://reviews.apache.org/r/55742/diff/5/?file=1613790#file1613790line1588> > > > > Won't flipping the interrupt bit cause problems with sending profile > > messages below?
Bruce and I looked through the code following (including that send method) and it looked like it handled interrupts properly. My thinking is that we can get sent an interrupt anytime, including during shutdown, and I would rather expect other code to handle that situation properly than defensively (and imperfectly) code around it. > On Jan. 24, 2017, 10:32 p.m., Anthony Baker wrote: > > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/AcceptorImplObserver.java, > > line 23 > > <https://reviews.apache.org/r/55742/diff/5/?file=1613791#file1613791line23> > > > > 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. I like the interface idea; I'm going to make `AcceptorImplObserver` an interface and attach it to the `AcceptorImpl` class. And make it thread-safe while I'm at it. - Galen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55742/#review162862 ----------------------------------------------------------- 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 > >
