You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Galen O'Sullivan <go...@pivotal.io> on 2017/01/20 21:43:43 UTC
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 20, 2017, 9:43 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
Diff: https://reviews.apache.org/r/55742/diff/
Testing (updated)
-------
precheckin in progress.
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
> 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
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
> 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
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review162512
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java (line 1577)
<https://reviews.apache.org/r/55742/#comment233824>
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?
- Bruce Schuchardt
On Jan. 20, 2017, 9:43 p.m., Galen O'Sullivan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55742/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 9:43 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
>
> Diff: https://reviews.apache.org/r/55742/diff/
>
>
> Testing
> -------
>
> precheckin in progress.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
> On Jan. 20, 2017, 11:37 p.m., Bruce Schuchardt wrote:
> > geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java, line 1578
> > <https://reviews.apache.org/r/55742/diff/3/?file=1611029#file1611029line1578>
> >
> > Something more like
> > boolean ignoredInterrupt = false;
> > .
> > .
> > .
> > } catch (InterruptedException ignore) {
> > ignoredInterrupt = true;
> > }
> > _finish shutting down other pool_
> > if (ignoredInterrupt) {
> > Thread.currentThread().interrupt();
> > }
> > return;
I've added test hooks to `AcceptorImpl.close()` and made a test that will avoid short-circuiting when interrupted. I'm planning to leave the interrupt code as is unless you have a strong objection.
- Galen
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review162521
-----------------------------------------------------------
On Jan. 20, 2017, 11:20 p.m., Galen O'Sullivan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55742/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 11:20 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
>
> Diff: https://reviews.apache.org/r/55742/diff/
>
>
> Testing
> -------
>
> precheckin in progress.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review162521
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java (line 1578)
<https://reviews.apache.org/r/55742/#comment233831>
Something more like
boolean ignoredInterrupt = false;
.
.
.
} catch (InterruptedException ignore) {
ignoredInterrupt = true;
}
_finish shutting down other pool_
if (ignoredInterrupt) {
Thread.currentThread().interrupt();
}
return;
- Bruce Schuchardt
On Jan. 20, 2017, 11:20 p.m., Galen O'Sullivan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55742/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2017, 11:20 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
>
> Diff: https://reviews.apache.org/r/55742/diff/
>
>
> Testing
> -------
>
> precheckin in progress.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review162856
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java (line 1536)
<https://reviews.apache.org/r/55742/#comment234220>
I moved this inside the `synchronized` block because if it isn't, the code inside the block could potentially be run twice. I'm not sure the effects of that, but it seems unexpected.
(consider a thread that pauses on the `synchronized` block while another is inside shutting down.)
- Galen O'Sullivan
On Jan. 24, 2017, 9:53 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, 9:53 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 in progress.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
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
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Udo Kohlmeyer <uk...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review163664
-----------------------------------------------------------
Ship it!
Ship It!
- Udo Kohlmeyer
On Jan. 30, 2017, 7:44 p.m., Galen O'Sullivan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55742/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2017, 7:44 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/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55742/diff/
>
>
> Testing
> -------
>
> precheckin started.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Bruce Schuchardt <bs...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/#review163589
-----------------------------------------------------------
Ship it!
Ship It!
- Bruce Schuchardt
On Jan. 30, 2017, 7:44 p.m., Galen O'Sullivan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55742/
> -----------------------------------------------------------
>
> (Updated Jan. 30, 2017, 7:44 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/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55742/diff/
>
>
> Testing
> -------
>
> precheckin started.
>
>
> Thanks,
>
> Galen O'Sullivan
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 30, 2017, 7:44 p.m.)
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
whoops, typo.
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 (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java 060683de6
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/55742/diff/
Testing
-------
precheckin started.
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 30, 2017, 7:40 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 (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java 060683de6
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/55742/diff/
Testing
-------
precheckin started.
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 30, 2017, 7:33 p.m.)
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
* Refactor `AcceptorImpl.close()` into multiple methods. This is just for readability at this point.
* Keep private variables private -- `AcceptorImplObserver` is no more. Instead, we put a public verification method on the `AcceptorImpl` itself.
* DUnit test for `AcceptorImpl.close()`.
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 (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java 060683de6
geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImplDUnitTest.java PRE-CREATION
Diff: https://reviews.apache.org/r/55742/diff/
Testing (updated)
-------
precheckin started.
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
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.
Changes
-------
Make AcceptorImplObserver an interface, fix threading issues.
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 (updated)
-----
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
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
> 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
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Anthony Baker <ab...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
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.
Changes
-------
Fix a lack of null check for the observer.
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 (updated)
-----
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 (updated)
-------
precheckin passed on (3), will run on (5).
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 24, 2017, 9:53 p.m.)
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
Add `AcceptorImplObserver` for a Dunit test.
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 (updated)
-----
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 in progress.
Thanks,
Galen O'Sullivan
Re: Review Request 55742: [GEODE-2324] and what look like a few other
bugs in the same method.
Posted by Galen O'Sullivan <go...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55742/
-----------------------------------------------------------
(Updated Jan. 20, 2017, 11:20 p.m.)
Review request for geode, Bruce Schuchardt, Hitesh Khamesra, and Udo Kohlmeyer.
Changes
-------
Interrupt the thread after catching InterruptedException.
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 (updated)
-----
geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/AcceptorImpl.java 060683de6
Diff: https://reviews.apache.org/r/55742/diff/
Testing
-------
precheckin in progress.
Thanks,
Galen O'Sullivan