C4Socket static hashmap entries

[dtchepak]

Hi!

I've been doing some profiling to try to find out what's keeping some instances alive. I think I've tracked down one culprit: I can find where C4Socket static hashmap entries get added, but not where they get removed. This is fairly new to me on this platform so my apologies if I have this wrong, but thought I'd raise it in case it was helpful.

C4Socket.java:

    // Map between SocketFactory Context and SocketFactory Class
    public static final Map<Object, Class> SOCKET_FACTORY
        = Collections.synchronizedMap(new HashMap<>());

    // Map between SocketFactory Context and Replicator
    public static final Map<Object, Replicator> SOCKET_FACTORY_CONTEXT
        = Collections.synchronizedMap(new HashMap<>());

AbstractReplicator.java:

C4Socket.SOCKET_FACTORY_CONTEXT.put(this, (Replicator) this);

Replicator.java:

    void initSocketFactory(Object socketFactoryContext) {
        C4Socket.SOCKET_FACTORY.put(socketFactoryContext, CBLWebSocket.class);
    }

Is there a way to remove these references once no longer needed? Or replace with WeakReferences?

I started looking into this because I could not tell what was referencing an instance I have. Profiling indicated the Replicator keeps alive the ReplicatorConfiguration which keeps alive the push filter which closes over some variables from that instance.

[bmeike]

What version of CB-Lite are you using?

[dtchepak]

@bmeike 2.6.0

[dtchepak]

Can reproduce the symptom with an app that triggers one-time replication on button press. Profiling app, triggering a few replications, forcing GC using profiler, then doing a heap dump in profiler should show multiple Replicator objects still live.

[bmeike]

What is it that is staying "alive" that you believe should not be? You say "instances"? Instances of what? Those two maps that you cite are essential for keeping references to objects used by replicators. It is essential that they NOT be garbage collected, simply because they are no longer referenced in the Java code. I think the first thing to do, here, is determine whether there actually is something that is not needed, that is being held in memory. There are definitely things being held in memory by those maps. We need to determine, though, whether they are necessary or not.

[dtchepak]

Hi @bmeike, Thanks for your reply.

The instances are of type Replicator (which in turn keeps a ReplicatorConfiguration instance, which keeps a Database instance and any push/pull filters). These are kept after the replicator status reaches STOPPED state, and after forcing GC. This is for one-shot replicator configurations (not continuous).

Are Replicator instances meant to be reused from client code? Due to the readonly config I'm creating a new one each time as I sometimes need to update the auth token used. If this is incorrect please let me know!

I will try to get a reproduction pushed up for you to take a look at. As far as I can tell Replicator instances and associated info will never get garbage collected for the life of the application. I may be mistaken about this, but I think it's worth checking to make sure these things can be cleaned up once no longer necessary. 😄

[dtchepak]

I've attempted to add a repro case here: https://github.com/dtchepak/android-workshop/tree/acd5551e7892b42d2d05938273280df0dc559f99.

Steps:

• Start app and begin profiling.
• Tap "CBL Example" button.
• Navigate back without starting replication.
• In profiler, force GC then do a heap dump. Observe that heap does not contain any CBL Database or Replicator objects.
• Tap "CBL Example" button.
• Enter syncgateway details and start replication, and wait for it to stop again as indicated by status (it does not matter if you don't put in valid replication details, just attempting to hit the end point is sufficient).
• Do the replication a total of x times.
• Navigate back to main activity. All CBL objects should now be eligible for GC.
• In profiler, force GC then do a heap dump. Observe that heap contains x number of Replicator instances, and also a Database instance that is kept alive by the Replicator instance. Compare to earlier step where Database instances were cleaned up as expected after exiting the activity without starting a Replicator.

Example of running 7 replications (and allowing to stop):

Please let me know if you need more info.

[bmeike]

The instances are of type Replicator (which in turn keeps a ReplicatorConfiguration instance, which keeps a Database instance and any push/pull filters). These are kept after the replicator status reaches STOPPED state, and after forcing GC. This is for one-shot replicator configurations (not continuous).

Are Replicator instances meant to be reused from client code? Due to the readonly config I'm creating a new one each time as I sometimes need to update the auth token used. If this is incorrect please let me know!

Yes. Exactly. This is not a bug. Until the Replicator reaches a stopped state, it, its configuration, and the Database to which it refers must be kept available. They can be invoked, asynchronously, at any time (until the replicator is stopped) by the native code. This is not a leak. This is correct behavior.

I will check to see if there is a way that the references could be held after the STOPPED state is reached.

[bmeike]

You are correct. This is a pretty horrible leak. Thanks for pointing it out.

Tracking in https://issues.couchbase.com/browse/CBL-404

[bmeike]

This bug has been fixed. Code is in the master branch. Closing.

[dtchepak]

@bmeike Thanks! The change looks awesome 👍

Do you have any idea of when this will be in a release? (rough ballpark, like weeks vs months?)

[bmeike]

Not my baliwick. I'd bet more like months...