Re: Review Request 49102: WAN Ack reader thread needs to be shut down before sending a close connection

2016-07-07 Thread anilkumar gingade 

---
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
 


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

2016-07-06 Thread Jason Huynh 

---
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

2016-06-30 Thread nabarun nag 


> 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
> > 
> >
> > 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

2016-06-30 Thread nabarun nag 

---
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)


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
> 
>

Re: Review Request 49102: WAN Ack reader thread needs to be shut down before sending a close connection

2016-06-30 Thread Jason Huynh 


> 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

2016-06-29 Thread anilkumar gingade 

---
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
> 
>

Review Request 49102: WAN Ack reader thread needs to be shut down before sending a close connection

2016-06-22 Thread Jason Huynh 

---
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