Re: [Dpp-devel] Review Request: Cleanup SarosPacketCollector and cancel collector if capacity reached

[Michael <ju...@in...>]

> On 2010-11-24 13:21:52.363031, Andreas Haferburg wrote:
> > Exactly when do we expect this limit to be reached? Can this happen when the archive received during the invitation is too big? And how big is too big? Couldn't we check this before sending the archive?
> 
> Michael wrote:
>     That is the point. I had a log.debug() for a month now and it is never reached and actually we do not expect it to reach ever (there is never more than 1 packet because it is only used for the invitation process). If it did it would be an error that should inform the application, nothing else.
>     I also thought about putting a lower limit or whether it needs a complete rework respective to it's usage (invitation process), however, my main purpose for the moment was to remove the redundant synchronized. Suggestions?
> 
> Andreas Haferburg wrote:
>     Laut javadoc von processPacket wird die Methode nur von einem thread aufgerufen, deshalb ist das synchronized unnötig, richtig? Falls ja -> +1.

The JavaDoc refers the Smack API processPacket - we use in XMPPTransmitter.prepareConnection(), forwarding it to the XMPPReceiver - everywhere else we use the same method in a different context. 
However, we use an ExecuterService in XMPPTransmitter.dispatThread (DispatchThreadContext singleton) but it's single threaded. Still there are two ways of communication in Saros (mind bytestreams), look at XMPPReceiver, still again the same dispatch singleton is used. (Well, just now I noticed that it actually is single threaded always ... what a mess, any synchronized there never made sense.)

But please note that there would be no need to be single threaded: SarosPacketCollector uses a LinkedBlockingQueue, from the JavaDoc of BlockingQueue:
"BlockingQueue implementations are thread-safe"

It seems the dispatchThread could be multi-threaded without problems - if there is no race problem afterwards in the SWT thread as you explained me once.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://saros-build.imp.fu-berlin.de/reviews/r/139/#review312
-----------------------------------------------------------


On 2010-11-22 19:12:25.369653, Michael wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://saros-build.imp.fu-berlin.de/reviews/r/139/
> -----------------------------------------------------------
> 
> (Updated 2010-11-22 19:12:25.369653)
> 
> 
> Review request for All Saros.
> 
> 
> Summary
> -------
> 
> The SarosPacketCollector had 2 unused methods that are removed (code smell of unused generality, furthermore the methods had shortcomings respective LinkedBlockingQueue usage)
> Furthermore the capacity mechanic is changed as it did not notify any client about a full queue nor it took any action (so even if something went wrong we got never to know). This also removes an expensive synchronized on process packet.
> 
> If the queue's capacity is reached now, the collector is canceled (will not queue any more packets) as it is supposed to be an error and the application should take respective actions. For convienience the next poll on an empty, canceled collector will throw an IllegalStateException to notify the client.
> Like this, a takeResult() function without timeout does not make sense, users of SarosPacketCollector should always specify a timeout because for other porposes Saros uses custom PacketListener implementations or re-polls (like in XMPPTransmitter.receive() method).
> 
> Please note that the SarosPacketCollector is only used during the invitation process.
> 
> 
> Diffs
> -----
> 
>   /src/de/fu_berlin/inf/dpp/net/internal/SarosPacketCollector.java 2730 
> 
> Diff: http://saros-build.imp.fu-berlin.de/reviews/r/139/diff
> 
> 
> Testing
> -------
> 
> Tested with 3 peers, invitation, invitation cancelling from host and from peer.
> 
> 
> Thanks,
> 
> Michael
> 
>