> 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?
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) {
}
```
- 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
>
>
