Fix ProGuard issue with map.keySet() on Java 8

[markuspfeiffer]

This fixes a ProGuard warning that occurs on Java 8, due to the
change of return type of Map.keySet(). This warning causes the build to
fail. See output attached below.

This issue is fixed by using Map.entrySet() rather than .keySet(). Use of
entrySet() is also preferable when both key and value are required, as
is the case here.

In addition, Iterator.remove() is used rather than Map.remove(), which
is the correct way of removing an entry while iterating. See
https://docs.oracle.com/javase/8/docs/api/java/util/Map.html#keySet--

If the map is modified while an iteration over the set is in progress
(except through the iterator's own remove operation), the results of
the iteration are undefined.

---

Warning: io.sqlc.SQLitePlugin: can't find referenced method 'java.util.concurrent.ConcurrentHashMap$KeySetView keySet()' in library class java.util.concurrent.ConcurrentHashMap
Warning: there were 1 unresolved references to library class members.
         You probably need to update the library versions.
         (http://proguard.sourceforge.net/manual/troubleshooting.html#unresolvedlibraryclassmember)
Warning: Exception while processing task java.io.IOException: Please correct the above warnings first.

[markuspfeiffer]

Guess I figured out why the loop was written as is. This seems aimed to ensure that values that are added while onDestroy() is already running are also removed. The downside is that it creates a new iterator for each value in the map.

This fails for values that are added just after Map.isEmpty() returns true, so still a (minor) race condition.

I feel it would be cleaner to reject new values as soon as onDestroy is called and either log a warning or throw an exception.

[brodycj]

Thanks @markuspfeiffer for the contribution, my apology for the delay.

I think the problem with the keySet() member function should be solved by storing as Map<> instead of ConcurrentHashMap in 4c7b59c as discussed in https://gist.github.com/AlainODea/1375759b8720a3f9f094, pointed out in #727.

I would say that you are right about the possible minor race condition. In general onDestroy means that the app is shutting down and should flush any remaining data, no more database files should be opened. For future enhancement, adding to #685 (upcoming minor release items).

In terms of extra iterator resources I would not be so concerned since this would be very small memory resource per database file and would only be generated upon internal app shutdown. I hope to get a chance to add some comments about this to the source code.

[markuspfeiffer]

I agree, creating multiple iterators is probably of no concern, I was just confused by the somewhat unusual way of removing those values :) Also agreed on the race condition.

As I pointed out, since the method is using both key and value, using entrySet() seems more natural and at least on Android Lint will also recommend it. That should not prevent a change to the more generic Map, so those changes could possibly be combined.

As I understand it, this will be fixed by #685? If so, we can close this MR from my point of view.

[brodycj]

I think the problem with the keySet() member function should be solved by storing as Map<> instead of ConcurrentHashMap in 4c7b59c as discussed in https://gist.github.com/AlainODea/1375759b8720a3f9f094, pointed out in #727.

Now part of 2.1.3 and 2.1.4

As I understand it, this will be fixed by #685? If so, we can close this MR from my point of view.

Yes, closing now. Please report here or preferably in a new issue if you find anything else. Thanks for contributing!

FYI I am still keeping #685 open to track anything else that needs to be fixed in the near term. Next will probably be an update to support cordova-windows@7 ref: #729

[brodycj]

Not closing yet, the following fix is still needed:

[...]
I feel it would be cleaner to reject new values as soon as onDestroy is called and either log a warning or throw an exception.

[...]
In general onDestroy means that the app is shutting down and should flush any remaining data, no more database files should be opened. For future enhancement, adding to #685 (upcoming minor release items).