You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jason Huynh <hu...@gmail.com> on 2016/06/22 17:52:15 UTC
Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/
-----------------------------------------------------------
Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
Repository: geode
Description
-------
When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
Diff: https://reviews.apache.org/r/49102/diff/
Testing
-------
Thanks,
Jason Huynh
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/#review141188
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java
<https://reviews.apache.org/r/49102/#comment206609>
How about adding comment about why we need to proceed even if the processor is stopped...This will help in future, if someone introduces the stop check back again...
- anilkumar gingade
On July 6, 2016, 8:42 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49102/
> -----------------------------------------------------------
>
> (Updated July 6, 2016, 8:42 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
> The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
> geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
> geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/WANTestBase.java 358ffaf
>
> Diff: https://reviews.apache.org/r/49102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by Jason Huynh <hu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/
-----------------------------------------------------------
(Updated July 6, 2016, 8:42 p.m.)
Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
Changes
-------
Previous changes caused a few dunit failures. This one removed the offending code.
Repository: geode
Description
-------
When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
Diffs (updated)
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
geode-wan/src/test/java/com/gemstone/gemfire/internal/cache/wan/WANTestBase.java 358ffaf
Diff: https://reviews.apache.org/r/49102/diff/
Testing
-------
Thanks,
Jason Huynh
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by Jason Huynh <hu...@gmail.com>.
> On June 29, 2016, 9:02 p.m., anilkumar gingade wrote:
> > Nice to have unit test for this.
Agreed, however this would require quite a few test hooks in product code. There are multiple scenarios that can cause this issue. I've inserted a "test case" to the ticket that will reproduce the issue without test hooks but it would not really be a test I would want to check in.
The problem is that there are multiple scenarios that can cause this to occur. Three different threads that would need to have test hooks to sync each other at specific points. The ack reader thread can be stuck on a socket read() or it can have already processed the header for the read, or it could be interrupted right before the read or it could be dead. All the while the dispatcher thread could be running and spawn a new ack reader thread (if it were dead) on a new connection or sending off a new batch. The gateway stopper thread could possibly send a close connectio on the old socket or if the ack reader thread was recreated, it would send on the new socket.
All those scenarios now are invalid... we have changed the shut down to shutdown the dispatcher thread (not allowing it to spawn a new ack reader thread if it's shutting down). The connection is also being closed which will force the ack reader to break out of the read() or prevent it from reading anything else from the input stream. All the new test hooks would be added and some of these scenarios are now unable to be hit.
- Jason
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/#review140051
-----------------------------------------------------------
On June 22, 2016, 5:52 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49102/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 5:52 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
> The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
> geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
>
> Diff: https://reviews.apache.org/r/49102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by anilkumar gingade <ag...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/#review140051
-----------------------------------------------------------
Ship it!
Nice to have unit test for this.
- anilkumar gingade
On June 22, 2016, 5:52 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49102/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 5:52 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
> The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
> geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
>
> Diff: https://reviews.apache.org/r/49102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by nabarun nag <nn...@pivotal.io>.
> On June 30, 2016, 11:14 p.m., nabarun nag wrote:
> > geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java, line 303
> > <https://reviews.apache.org/r/49102/diff/1/?file=1427588#file1427588line303>
> >
> > Hi Jason,
> > While debugging this ticket we found out that the boolean startAckReaderThread is completely ignore by the method getConnection.
> > Should we make use of it in line 329
> > if (this.ackReaderThread == null || !this.ackReaderThread.isRunning())
> >
> > Or refactor the method signature.
Sorry, i dont know what the protocol is for "open" issue. Checked it by mistake and I can't edit it
- nabarun
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/#review140264
-----------------------------------------------------------
On June 22, 2016, 5:52 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49102/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 5:52 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
> The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
> geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
>
> Diff: https://reviews.apache.org/r/49102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>
Re: Review Request 49102: WAN Ack reader thread needs to be shut down
before sending a close connection
Posted by nabarun nag <nn...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/49102/#review140264
-----------------------------------------------------------
geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java (line 303)
<https://reviews.apache.org/r/49102/#comment205618>
Hi Jason,
While debugging this ticket we found out that the boolean startAckReaderThread is completely ignore by the method getConnection.
Should we make use of it in line 329
if (this.ackReaderThread == null || !this.ackReaderThread.isRunning())
Or refactor the method signature.
- nabarun nag
On June 22, 2016, 5:52 p.m., Jason Huynh wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49102/
> -----------------------------------------------------------
>
> (Updated June 22, 2016, 5:52 p.m.)
>
>
> Review request for geode, anilkumar gingade, Barry Oglesby, nabarun nag, Dan Smith, and xiaojian zhou.
>
>
> Repository: geode
>
>
> Description
> -------
>
> When closing a sender, the close connection message is sent on the same connection that is used by the ack reader thread. This causes an issue as two threads are now reading off the same socket concurrently. The fix is to prevent this from happening but to do so, the input stream needs to be closed (to free up from a socket read()).
> The dispatcher also needs to shut down before the close connection is sent out or it will spawn off another ack reader thread.
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/AbstractGatewaySenderEventProcessor.java ce08e8d
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/parallel/ConcurrentParallelGatewaySenderEventProcessor.java 07a3be5
> geode-core/src/main/java/com/gemstone/gemfire/internal/cache/wan/serial/ConcurrentSerialGatewaySenderEventProcessor.java ff810ec
> geode-wan/src/main/java/com/gemstone/gemfire/internal/cache/wan/GatewaySenderEventRemoteDispatcher.java b178192
>
> Diff: https://reviews.apache.org/r/49102/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jason Huynh
>
>