[jitsi-dev] Ice4j Patch


#1

Hi,
I've attached a patch with some fixes to ice4j.

We've done the following:

* Implemented support for ServerSocket in PseudoTcpSocketFactory. This is a hack but as you know from our previous discussion (and http://stackoverflow.com/questions/11982698/why-is-serversocket-setsocketfactory-static) there's probably no better way to solve it.

* Changed the use of the thread-pool in CandidateHarvesterSet so that it reuses the same thread-pool among different CandidateHarvesterSet-instances. This should improve the effects of the thread-pool since more threads are pooled together.

* Fixed a synchronization issue in UPNPHarvester. On some rare occasions it got stuck at rootSync.wait() because the threads finished between the finishThreads != 2 check and the synchronization on rootSync. I think a better long term solution for this would be to use a common thread-pool for all short tasks like this and then just wait for the execution to finish using Future.get(). That should reduce the number of threads created and reduce the risk of other sync-issue similar to this one.

* Made Connector interrupt its thread when stopping (sometimes it's seems get stuck in wait(), so closing the socket isn't enough). Correct me if I'm wrong about this because I was unable to recreate the problem, so I haven't been able to verify properly that my theory is right.

My IDE (IntelliJ) automatically re-organized some of the imports, which adds some noise to the patch. Sorry about that.

It would be great if you'd accept this patch, because it would allow me to continue working on the same version as you and hopefully be able to contribute with more stuff later on. Some of these changes are required for my company's project so if you don't want to accept the patch then I'm forced to fork the project and work on a separate branch.

Kind Regards
Carl Hasselskog
Degoo Backup AB
carl@degoo.com<mailto:carl@degoo.com>
Phone: +46 73 070 1821
http://degoo.com<http://degoo.com/>
http://twitter.com/#!/CarlHasselskog

Changes.patch (7.68 KB)


#2

Hello Carl,

Sorry for the delay, but I want to let you know that reviewing your patch is on my TODO list. I will review it as soon as possible, but unfortunately it may not happen before next month.

Thank you for submitting these fixes.

Regards,
Vincent

···

On 8/27/12 6:04 PM, Carl Hasselskog wrote:

Hi,
I've attached a patch with some fixes to ice4j.

We've done the following:

* Implemented support for ServerSocket in PseudoTcpSocketFactory. This is a hack but as you know from our previous discussion (and http://stackoverflow.com/questions/11982698/why-is-serversocket-setsocketfactory-static) there's probably no better way to solve it.

* Changed the use of the thread-pool in CandidateHarvesterSet so that it reuses the same thread-pool among different CandidateHarvesterSet-instances. This should improve the effects of the thread-pool since more threads are pooled together.

* Fixed a synchronization issue in UPNPHarvester. On some rare occasions it got stuck at rootSync.wait() because the threads finished between the finishThreads != 2 check and the synchronization on rootSync. I think a better long term solution for this would be to use a common thread-pool for all short tasks like this and then just wait for the execution to finish using Future.get(). That should reduce the number of threads created and reduce the risk of other sync-issue similar to this one.

* Made Connector interrupt its thread when stopping (sometimes it's seems get stuck in wait(), so closing the socket isn't enough). Correct me if I'm wrong about this because I was unable to recreate the problem, so I haven't been able to verify properly that my theory is right.

My IDE (IntelliJ) automatically re-organized some of the imports, which adds some noise to the patch. Sorry about that.

It would be great if you'd accept this patch, because it would allow me to continue working on the same version as you and hopefully be able to contribute with more stuff later on. Some of these changes are required for my company's project so if you don't want to accept the patch then I'm forced to fork the project and work on a separate branch.

Kind Regards
Carl Hasselskog
Degoo Backup AB
carl@degoo.com<mailto:carl@degoo.com>
Phone: +46 73 070 1821
http://degoo.com<http://degoo.com/>
http://twitter.com/#!/CarlHasselskog

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org


#3

Hello Carl,

Thank you for submitting these patches. I have begin to apply them, with some slightly changes:

Hi,
I've attached a patch with some fixes to ice4j.

We've done the following:

* Implemented support for ServerSocket in PseudoTcpSocketFactory. This is a hack but as you know from our previous discussion (and http://stackoverflow.com/questions/11982698/why-is-serversocket-setsocketfactory-static) there's probably no better way to solve it.

- I do not understand what you try to do here: you add some code to get a custom SocketFactory for ServerSocket, but you change the global ServerSocket SocketFactory too. Moreover, you use a PseudoTcpServerSocket class which is not defined anywhere.

* Changed the use of the thread-pool in CandidateHarvesterSet so that it reuses the same thread-pool among different CandidateHarvesterSet-instances. This should improve the effects of the thread-pool since more threads are pooled together.

- Ok. Just added the comment for the new attribute created.

* Fixed a synchronization issue in UPNPHarvester. On some rare occasions it got stuck at rootSync.wait() because the threads finished between the finishThreads != 2 check and the synchronization on rootSync. I think a better long term solution for this would be to use a common thread-pool for all short tasks like this and then just wait for the execution to finish using Future.get(). That should reduce the number of threads created and reduce the risk of other sync-issue similar to this one.

- Ok, done.

* Made Connector interrupt its thread when stopping (sometimes it's seems get stuck in wait(), so closing the socket isn't enough). Correct me if I'm wrong about this because I was unable to recreate the problem, so I haven't been able to verify properly that my theory is right.

- OK. The only instruction that may block is the "receive" function. That is why I prefer to close the socket (to remove the read lock) when stopping the Connector.

Regards,
Vincent

···

On 8/27/12 6:04 PM, Carl Hasselskog wrote:

My IDE (IntelliJ) automatically re-organized some of the imports, which adds some noise to the patch. Sorry about that.

It would be great if you'd accept this patch, because it would allow me to continue working on the same version as you and hopefully be able to contribute with more stuff later on. Some of these changes are required for my company's project so if you don't want to accept the patch then I'm forced to fork the project and work on a separate branch.

Kind Regards
Carl Hasselskog
Degoo Backup AB
carl@degoo.com<mailto:carl@degoo.com>
Phone: +46 73 070 1821
http://degoo.com<http://degoo.com/>
http://twitter.com/#!/CarlHasselskog

--
Vincent Lucas, Ph.D. Jitsi developer
chenzo@jitsi.org http://jitsi.org