[jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events. Adds


#1

Hey Damian

I think something went wrong with this new stuff. Ever since it is active, Jitsi doesn't shutdown when I do an automatic update to a newer build and I have to kill the run.exes manually.
Perhaps it's not this isolated commit, but it's for sure not more than a month since it started.

Regards,
Ingo

···

-----Original Message-----
From: damencho@java.net [mailto:damencho@java.net]
Sent: Freitag, 5. Oktober 2012 08:31
To: commits@jitsi.java.net
Subject: [jitsi~svn:9948] Updates sysactivity handling QUERY_ENDSESSION and
ENDSESSION events. Adds
Project: jitsi
Repository: svn
Revision: 9948
Author: damencho
Date: 2012-10-05 12:31:11 UTC
Link:

Log Message:
------------
Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events. Adds
bundle that listens for the new events to handle clean shutdown.

Revisions:
----------
9948


#2

Hi everyone,

I'm a student at CMU in CS and for one of our courses, we need to bring (my teammate and I) some contribution to an open source project.

We've followed the tutorial about how to retrieve the source code and configure Eclipse.

We just go through the code for the moment and try to understand the general architecture.

By doing so we found two pieces of code that we had difficulties to understand :

The first was in the class package net.java.sip.communicator.impl.gui.main.contactlist.AddContactDialog:

This class implements the ExportedWindow interface, and in some of its methods there seems to be an apparent infinite recursive loop (the method maximise() for example unconditionally calls itself, which would cause a stack overflow).

Is it possible to have a bit more explanations about this, I think we may be missing something important?

The second issue we ran into is situated in the class net.java.sip.communicator.impl.contactlist.MetaContactImpl :

There are a few boolean statements in if statement that seem more complicated that they should, for example

public Contact getContact(String contactAddress,
                               ProtocolProviderService ownerProvider)
     {
         for (Contact contact : protoContacts)
         {
             if(contact.getProtocolProvider() == ownerProvider
                && (contact.getAddress().equals(contactAddress)
                     >> contact.equals(contactAddress))) <=======================
                 return contact;
         }

         return null;
     }

Because the interface Contact does not define any equals(String) method, this equals will be resolved as a call to equal(Object), which is not defined for most of Contact's implementations. It will therefore be resolved to the equals method defined in java.lang.Object and always return false because contact and contactAddress or not the same type.

It doesn't bring any error here, but the code could be reduced to improve understanding.

Any feedback would be greatly appreciated

Thanks

Damien and Adrien


#3

Hi Ingo,

this commit only adds handling messages on windows shutdown or logout.
And doesn't handle anything on shutdown of the application. The only
significant change regarding shutdown is that we decrease the waiting
time before System.exit call, and this is only if for some reason a
bundle is blocking the stopping process. I noticed a small check to
add there, which I'm committing now. I don't think it is your case,
cause you are not using that system property where that code is.
So I believe the problem is somewhere else. Can you attach if possible
a thread dump when such stop don't succeed?

Thanks
damencho

···

On Sat, Oct 20, 2012 at 9:56 PM, Ingo Bauersachs <ingo@jitsi.org> wrote:

Hey Damian

I think something went wrong with this new stuff. Ever since it is active, Jitsi doesn't shutdown when I do an automatic update to a newer build and I have to kill the run.exes manually.
Perhaps it's not this isolated commit, but it's for sure not more than a month since it started.

Regards,
Ingo

-----Original Message-----
From: damencho@java.net [mailto:damencho@java.net]
Sent: Freitag, 5. Oktober 2012 08:31
To: commits@jitsi.java.net
Subject: [jitsi~svn:9948] Updates sysactivity handling QUERY_ENDSESSION and
ENDSESSION events. Adds
Project: jitsi
Repository: svn
Revision: 9948
Author: damencho
Date: 2012-10-05 12:31:11 UTC
Link:

Log Message:
------------
Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events. Adds
bundle that listens for the new events to handle clean shutdown.

Revisions:
----------
9948


#4

The first was in the class package net.java.sip.communicator.impl.gui.main.contactlist.AddContactDialog:

This class implements the ExportedWindow interface, and in some of its methods there seems to be an apparent infinite recursive loop (the method maximise() for example unconditionally calls itself, which would cause a stack overflow).

Is it possible to have a bit more explanations about this, I think we may be missing something important?

You're right. The same goes for minimize().

Devs, do we want to maximize/minimize the AddContactDialog or do we want to disable them like we do for some other dialogs?

The second issue we ran into is situated in the class net.java.sip.communicator.impl.contactlist.MetaContactImpl :

There are a few boolean statements in if statement that seem more complicated that they should, for example

public Contact getContact(String contactAddress,
                              ProtocolProviderService ownerProvider)
    {
        for (Contact contact : protoContacts)
        {
            if(contact.getProtocolProvider() == ownerProvider
               && (contact.getAddress().equals(contactAddress)
                    >> contact.equals(contactAddress))) <=======================
                return contact;
        }

        return null;
    }

Because the interface Contact does not define any equals(String) method, this equals will be resolved as a call to equal(Object), which is not defined for most of Contact's implementations. It will therefore be resolved to the equals method defined in java.lang.Object and always return false because contact and contactAddress or not the same type.

It doesn't bring any error here, but the code could be reduced to improve understanding.

You're wrong. For example, ContactJabberImpl implements #equals(Object) and explicitly supports String.

Devs, it's a totally different matter but ContactJabberImpl uses equalsIgnoreCase on getAddress() and defines hashCode to be getAddress().hashCode() which returns different hashCodes for value equal instances. Such a break of the general contract for the hashCode method may lead to unexpected behavior when hash-aware collections are used. Anyway, we likely don't rely on such operations and I'm mentioning it just in case.

···

On 20.10.2012, at 23:00, Damien Engels <dengels@andrew.cmu.edu> wrote:


#5

Hi

Thanks for looking at it!
As I thought, it wasn't just this one commit, but something is definitely
wrong. Threaddump is attached as requested, this time it hang during a
File->Quit operation (but it was still build 4288.10004)

It's a long time ago, but I remember that System.exit may cause a deadlock
inside Felix/OSGi if used incorrectly...

Regards,
Ingo

From: damencho@damencho.com [mailto:damencho@damencho.com] On Behalf Of
Damian Minkov
Sent: Montag, 22. Oktober 2012 02:29
To: dev@jitsi.java.net
Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling
QUERY_ENDSESSION and ENDSESSION events. Adds
Hi Ingo,

this commit only adds handling messages on windows shutdown or logout.
And doesn't handle anything on shutdown of the application. The only
significant change regarding shutdown is that we decrease the waiting
time before System.exit call, and this is only if for some reason a
bundle is blocking the stopping process. I noticed a small check to
add there, which I'm committing now. I don't think it is your case,
cause you are not using that system property where that code is.
So I believe the problem is somewhere else. Can you attach if possible
a thread dump when such stop don't succeed?

Thanks
damencho

Hey Damian

I think something went wrong with this new stuff. Ever since it is

active,

Jitsi doesn't shutdown when I do an automatic update to a newer build and

I

have to kill the run.exes manually.

Perhaps it's not this isolated commit, but it's for sure not more than
a month since it started.

Regards,
Ingo

Repository: svn
Revision: 9948
Author: damencho
Date: 2012-10-05 12:31:11 UTC
Link:

Log Message:
------------
Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events.

Adds

threaddump.txt (43.1 KB)

···

-----Original Message-----
On Sat, Oct 20, 2012 at 9:56 PM, Ingo Bauersachs <ingo@jitsi.org> wrote:

damencho@java.net wrote on 2012-10-05: >>> ENDSESSION events. Adds >>> Project: jitsi

bundle that listens for the new events to handle clean shutdown.

Revisions:
----------
9948


#6

From: Lubomir Marinov [mailto:lubo@sip-communicator.org]

The first was in the class package
net.java.sip.communicator.impl.gui.main.contactlist.AddContactDialog:

This class implements the ExportedWindow interface, and in some of its
methods there seems to be an apparent infinite recursive loop (the method
maximise() for example unconditionally calls itself, which would cause a
stack overflow).

Is it possible to have a bit more explanations about this, I think we may
be missing something important?

You're right. The same goes for minimize().

Devs, do we want to maximize/minimize the AddContactDialog or do we want

to

disable them like we do for some other dialogs?

Without looking at the source, the Add Contact dialog is already fixed size
on Windows. Do I miss something?

[snip]

You're wrong. For example, ContactJabberImpl implements #equals(Object)

and

explicitly supports String.

This itself is a problem because it breaks the symmetry-contract of #equals
because String.equals(jabberContact) will never return true. Mathematically
it has to be reflexive, symmetric, transitive - see JavaDoc for examples.

Devs, it's a totally different matter but ContactJabberImpl uses
equalsIgnoreCase on getAddress() and defines hashCode to be
getAddress().hashCode() which returns different hashCodes for value equal
instances. Such a break of the general contract for the hashCode method

may

lead to unexpected behavior when hash-aware collections are used. Anyway,

we

likely don't rely on such operations and I'm mentioning it just in case.

I know that we have higher-priority issues, but stuff like that should be
fixed rather sooner than later. Someone might start using it somewhere, has
inexplicable problems and scratches his head out while searching for hours
if not even days until finding out that it's the overridden
equals/hashCode...

Regards,
Ingo

···

On 20.10.2012, at 23:00, Damien Engels <dengels@andrew.cmu.edu> wrote:


#7

Hey,

From: Lubomir Marinov [mailto:lubo@sip-communicator.org]

The first was in the class package
net.java.sip.communicator.impl.gui.main.contactlist.AddContactDialog:

This class implements the ExportedWindow interface, and in some of its
methods there seems to be an apparent infinite recursive loop (the method
maximise() for example unconditionally calls itself, which would cause a
stack overflow).

Is it possible to have a bit more explanations about this, I think we may
be missing something important?

You're right. The same goes for minimize().

Devs, do we want to maximize/minimize the AddContactDialog or do we want

to

disable them like we do for some other dialogs?

Without looking at the source, the Add Contact dialog is already fixed size
on Windows. Do I miss something?

It is fixed size yes! I've just disabled maximise/minimise for this window, as Lubo suggested.

[snip]

You're wrong. For example, ContactJabberImpl implements #equals(Object)

and

explicitly supports String.

This itself is a problem because it breaks the symmetry-contract of #equals
because String.equals(jabberContact) will never return true. Mathematically
it has to be reflexive, symmetric, transitive - see JavaDoc for examples.

Devs, it's a totally different matter but ContactJabberImpl uses
equalsIgnoreCase on getAddress() and defines hashCode to be
getAddress().hashCode() which returns different hashCodes for value equal
instances. Such a break of the general contract for the hashCode method

may

lead to unexpected behavior when hash-aware collections are used. Anyway,

we

likely don't rely on such operations and I'm mentioning it just in case.

I know that we have higher-priority issues, but stuff like that should be
fixed rather sooner than later. Someone might start using it somewhere, has
inexplicable problems and scratches his head out while searching for hours
if not even days until finding out that it's the overridden
equals/hashCode…

Thanks for reporting! Fixed in r10008.

Cheers,
Yana

···

On Oct 20, 2012, at 11:05 PM, Ingo Bauersachs <ingo@jitsi.org> wrote:

On 20.10.2012, at 23:00, Damien Engels <dengels@andrew.cmu.edu> wrote:

Regards,
Ingo


#8

Hey

Not sure if Emil already mentioned/noted something internally, but just to
keep it for the record: it is indeed caused by some kind of deadlock inside
Felix' Shutdown-Hook.

The reason because this wasn't seen earlier is the reduced default
Shutdown-Timeout (was 15s and is now 3s). If a sufficiently large number of
accounts is set up (on my machine its 8), Jitsi doesn't have enough time to
properly disconnect them all in the 3s. Because of that the Shutdown-Plugin
kicks in and causes the deadlock.

IMO the timeout of 3s is too short, but also the deadlock needs to be
solved. One way to do that is Felix' property to disable the Shutdown-Hook:
felix.shutdown.hook=false
This shutdown hook causes Felix to cleanly shut down the framework (bundle
0) on a System.exit(), but as we're already doing that ourselves when
exiting, it is redundant.

Regards,
Ingo

From: Ingo Bauersachs [mailto:ingo@sip-communicator.org] On Behalf Of Ingo
Bauersachs
Sent: Montag, 22. Oktober 2012 19:20
To: dev@jitsi.java.net
Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling
QUERY_ENDSESSION and ENDSESSION events. Adds

Hi

Thanks for looking at it!
As I thought, it wasn't just this one commit, but something is definitely
wrong. Threaddump is attached as requested, this time it hang during a
File->Quit operation (but it was still build 4288.10004)

It's a long time ago, but I remember that System.exit may cause a deadlock
inside Felix/OSGi if used incorrectly...

Regards,
Ingo

> From: damencho@damencho.com [mailto:damencho@damencho.com] On Behalf Of
> Damian Minkov
> Sent: Montag, 22. Oktober 2012 02:29
> To: dev@jitsi.java.net
> Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling
> QUERY_ENDSESSION and ENDSESSION events. Adds
> Hi Ingo,
>
> this commit only adds handling messages on windows shutdown or logout.
> And doesn't handle anything on shutdown of the application. The only
> significant change regarding shutdown is that we decrease the waiting
> time before System.exit call, and this is only if for some reason a
> bundle is blocking the stopping process. I noticed a small check to
> add there, which I'm committing now. I don't think it is your case,
> cause you are not using that system property where that code is.
> So I believe the problem is somewhere else. Can you attach if possible
> a thread dump when such stop don't succeed?
>
> Thanks
> damencho
>
>
>> Hey Damian
>>
>> I think something went wrong with this new stuff. Ever since it is
active,
> Jitsi doesn't shutdown when I do an automatic update to a newer build

and

···

-----Original Message-----
> -----Original Message-----
> On Sat, Oct 20, 2012 at 9:56 PM, Ingo Bauersachs <ingo@jitsi.org> wrote:
I
> have to kill the run.exes manually.
>> Perhaps it's not this isolated commit, but it's for sure not more than
>> a month since it started.
>>
>> Regards,
>> Ingo
>>
>>
>> damencho@java.net wrote on 2012-10-05: > >>> ENDSESSION events. Adds > >>> Project: jitsi
>>> Repository: svn
>>> Revision: 9948
>>> Author: damencho
>>> Date: 2012-10-05 12:31:11 UTC
>>> Link:
>>>
>>> Log Message:
>>> ------------
>>> Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events.
Adds
>>> bundle that listens for the new events to handle clean shutdown.
>>>
>>>
>>> Revisions:
>>> ----------
>>> 9948
>>>
>>


#9

You're wrong. For example, ContactJabberImpl implements
#equals(Object) and explicitly supports String.

This itself is a problem because it breaks the symmetry-contract of

#equals

because String.equals(jabberContact) will never return true.

Mathematically

it has to be reflexive, symmetric, transitive - see JavaDoc for examples.

Devs, it's a totally different matter but ContactJabberImpl uses
equalsIgnoreCase on getAddress() and defines hashCode to be
getAddress().hashCode() which returns different hashCodes for value
equal instances. Such a break of the general contract for the hashCode
method may lead to unexpected behavior when hash-aware collections are
used. Anyway, we likely don't rely on such operations and I'm
mentioning it just in case.

I know that we have higher-priority issues, but stuff like that should be
fixed rather sooner than later. Someone might start using it somewhere,

has

inexplicable problems and scratches his head out while searching for

hours

if not even days until finding out that it's the overridden
equals/hashCode.

Thanks for reporting! Fixed in r10008.

Thanks Yana, but that fixes only the hashCode stuff and not that the
#equals-Contract is broken (the symmetry-stuff). I don't insist on fixing
that right now or for the 2.0 release, but at least let us make some further
note of this than just a note on the dev-list.

Cheers,
Yana

Regards,
Ingo

Regards,
Ingo


#10

I've just checked in a fix for this, so let me know if you still see problems.

The fix relies on just calling System.exit() when we choose to shut down and the felix shutdown hooks take care of stopping everything nicely.
I've replaced the System.exit() call in the shutdown-timeout plugin with Runtime.getRuntime().halt() which doesn't run the shutdown hooks in felix so can't be blocked.

We'll need to keep a close eye on this to ensure that we're not resorting to the halt() in the shutdown-plugin in the mainline case. In future I hope to add a thread dump to the log just before we call .halt() to help diagnose problems in this area.

(And there is still the issue that was reported in the threaddump earlier in this thread - The felix thread is stuck setting the tray icon in native code which is blocking shutdown...

"FelixStartLevel" daemon prio=6 tid=0x064e5c00 nid=0x9cc0 runnable [0x06a8f000]
   java.lang.Thread.State: RUNNABLE
  at sun.awt.windows.WTrayIconPeer.setNativeIcon(Native Method)
  at sun.awt.windows.WTrayIconPeer.createNativeImage(Unknown Source)
  at sun.awt.windows.WTrayIconPeer.updateNativeImage(Unknown Source)
  - locked <0x1550b918> (a sun.awt.windows.WTrayIconPeer)
  at sun.awt.windows.WTrayIconPeer.updateImage(Unknown Source)
  at java.awt.TrayIcon.setImage(Unknown Source)
  at sun.reflect.GeneratedMethodAccessor21.invoke(Unknown Source)
  at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
  at java.lang.reflect.Method.invoke(Unknown Source)
  at net.java.sip.communicator.impl.osdependent.TrayIcon$AWTTrayIconPeer.setIcon(TrayIcon.java:308)
)
Tom

···

-----Original Message-----

From: Ingo Bauersachs [mailto:ingo@sip-communicator.org] On Behalf Of Ingo Bauersachs

Sent: 22 November 2012 19:11
To: dev@jitsi.java.net
Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events. Adds

Hey

Not sure if Emil already mentioned/noted something internally, but just to keep it for the record: it is indeed caused by some kind of deadlock inside Felix' Shutdown-Hook.

The reason because this wasn't seen earlier is the reduced default Shutdown-Timeout (was 15s and is now 3s). If a sufficiently large number of accounts is set up (on my machine its 8), Jitsi doesn't have enough time to properly disconnect them all in the 3s. Because of that the Shutdown-Plugin kicks in and causes the deadlock.

IMO the timeout of 3s is too short, but also the deadlock needs to be solved. One way to do that is Felix' property to disable the Shutdown-Hook:
felix.shutdown.hook=false
This shutdown hook causes Felix to cleanly shut down the framework (bundle
0) on a System.exit(), but as we're already doing that ourselves when exiting, it is redundant.

Regards,
Ingo

-----Original Message-----
From: Ingo Bauersachs [mailto:ingo@sip-communicator.org] On Behalf Of
Ingo Bauersachs
Sent: Montag, 22. Oktober 2012 19:20
To: dev@jitsi.java.net
Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity handling
QUERY_ENDSESSION and ENDSESSION events. Adds

Hi

Thanks for looking at it!
As I thought, it wasn't just this one commit, but something is
definitely wrong. Threaddump is attached as requested, this time it
hang during a
File->Quit operation (but it was still build 4288.10004)

It's a long time ago, but I remember that System.exit may cause a
deadlock inside Felix/OSGi if used incorrectly...

Regards,
Ingo

> -----Original Message-----
> From: damencho@damencho.com [mailto:damencho@damencho.com] On Behalf
> Of Damian Minkov
> Sent: Montag, 22. Oktober 2012 02:29
> To: dev@jitsi.java.net
> Subject: [jitsi-dev] Re: [jitsi~svn:9948] Updates sysactivity
> handling QUERY_ENDSESSION and ENDSESSION events. Adds Hi Ingo,
>
> this commit only adds handling messages on windows shutdown or logout.
> And doesn't handle anything on shutdown of the application. The only
> significant change regarding shutdown is that we decrease the
> waiting time before System.exit call, and this is only if for some
> reason a bundle is blocking the stopping process. I noticed a small
> check to add there, which I'm committing now. I don't think it is
> your case, cause you are not using that system property where that code is.
> So I believe the problem is somewhere else. Can you attach if
> possible a thread dump when such stop don't succeed?
>
> Thanks
> damencho
>
>
> On Sat, Oct 20, 2012 at 9:56 PM, Ingo Bauersachs <ingo@jitsi.org> wrote:
>> Hey Damian
>>
>> I think something went wrong with this new stuff. Ever since it is
active,
> Jitsi doesn't shutdown when I do an automatic update to a newer
> build

and

I
> have to kill the run.exes manually.
>> Perhaps it's not this isolated commit, but it's for sure not more
>> than a month since it started.
>>
>> Regards,
>> Ingo
>>
>>
>> damencho@java.net wrote on 2012-10-05: > >>> ENDSESSION events. Adds > >>> Project: jitsi
>>> Repository: svn
>>> Revision: 9948
>>> Author: damencho
>>> Date: 2012-10-05 12:31:11 UTC
>>> Link:
>>>
>>> Log Message:
>>> ------------
>>> Updates sysactivity handling QUERY_ENDSESSION and ENDSESSION events.
Adds
>>> bundle that listens for the new events to handle clean shutdown.
>>>
>>>
>>> Revisions:
>>> ----------
>>> 9948
>>>
>>