removeChangeListener lock

[RocketRider]

Calling removeChangeListener from the main thread (e.g. onPause) can lead to not responsive UI when an other query in the background locks the DB. Could there be a different lock used for the change listeners in the AbstractDatabase, so they would not need to wait for long querys/tasks?

Otherwise I will need to run the removeChangeListener async.

[bmeike]

That should not be true, in general. The lock used by removeChangeListener is used, with one exception, only to control access to the list of listeners. The exception is that that same lock is held during calls to ChangeListener.changed. If your listener is slow and is running on the UI thread (the default) then it will hang removeChangeListener. You might consider specifying a non-null executor in your call to addChangeListener

[RocketRider]

I looked into the code of AbstractDatabase.java and I am not sure I am seeing what you are describing. The removeChangeListener synchronizes on "lock". The methods getCount, getDocument, save, delete,... are using the same "lock" instance.

When I get a change I only get the document ID, so I call the change listener in the executor service and acquire the document in the background will lock the "lock" object. And now the other code running in the UI thread wants to close the old activity and remove its listener will have to wait for the lock.

[bmeike]

You are exactly correct about the use of AbstractDatabase.lock. I was unclear on to which of the several removeChangeListener methods you were referring.

Any use of a native method must be synchronized on a single lock. Them's the rules.

The implication is that you cannot use methods that call a native method, from the UI thread.

We should do a better job of noting such methods in our documentation. A safe assumption, though, is that that includes all of the methods on Database.

[bmeike]

@RocketRider , does this explain things? Do you have further questions?

[RocketRider]

I understand your arguments but I think this leads to ugly and unsecure code in the activities/fragments.

I have fragments which want to listen to DB Changes, so they register a change listener. I will listen to the changes in the background, that is fine. But I will need to remove the change listener from the DB so it gets correctly garbage collected. So I will do this in the onPause or destroy of the fragment. After the destroy method all the listeners should be removed. So I should call removeChangeListener synchronous. But if I do so your code could block the UI Thread for longer time and the user will see a freeze. With your Implementation I would be forced to call the removeChangeListener async. This could lead to ugly run time problems as the change listener could fire after the fragment was destroyed. Additionally this could result in memory leaks very easily.

[bmeike]

I have some suggestions:

• Perhaps it is not your fragments that should be listening to the DB? Perhaps this would be easier to architect if your ViewModel (Presenter, etc) listened to the DB?
• There is no particular reason, that I can see, to call removeChangeListener synchronously, if the view no longer responds to messages that it receives. Those messages are inherently asynchronous. Removing the listener does not guarantee that previously enqueued messages will not be delivered.

Unfortunately removing the change listener changes the state of multi-threaded native code. In order to do that safely, the state must be protected by a single lock.

[RocketRider]

Thank you again. Yes the ViewModel approach is the best way to do it. A note in the documentation would be great to remind developers about this.