CXF-7710: ClientImpl is memory-leak prone

[facundovs]

No description provided.

[deki]

Thanks for the PR.

According to Javadoc https://docs.oracle.com/javase/8/docs/api/java/lang/Thread.html: Every thread has a name for identification purposes. More than one thread may have the same name.

So using name seems not to be a good idea.

[facundovs]

Hi @deki . What do you think about using thread id instead?

[dkulp]

Nothing about this change will actually work. Changing to a WeakHashMap<String, ....> and using a "new String(...)" for the key will result in nothing actually holding onto the key and the context could be garbage collected immediately. The test case you added (requestContextIsGarbageCollected) shows the exact problem. Two calls to getRequestContext() on the same thread should return the same context. It's very common to do:

client.getRequestContext().put("A", aValue);
client.getRequestContext().put("B", bValue);
client.getRequestContext().put("C", cValue);

and that NEEDS to work. If something triggers a GC between there, values will get lost.

As part of CXF-7591, we made sure the getResponseContext().clear() will work so that users can explicitly clear the context when done with it to avoid this problem.

[dkulp]

I think adding a method to Client like:

    default AutoCloseable autoCloseContexts() {
        return new AutoCloseable() {
            @Override
            public void close() throws Exception {
                getRequestContext().clear();
                getResponseContext().clear();
            }
        };
    }

might be a useful idea. In the code, you could use the try with resources to make sure the contexts are completely cleared:

try (((Client)proxy).autoCloseContexts()) {
   proxy.callWhateverMethods(....);
}

Thoughts?

[rmannibucau]

you probably want to use the thread id and not the name which doesn't have to be unique?

[johnament]

Also, you're not going to want to new the string each time, for performance reasons. Can the methods that call this reduce the number of times they call it?

[rmannibucau]

Why not closing the client for such cases if issue is the threadlocals, most of them can be cleaned up once used and re-initialied from the client at next request no? It avoids new API.

[dkulp]

Closing the entire client is very different (which we already support). When close on the client is called, EVERYTHING gets shutdown and cleaned up including conduit connections, pointers to the WSDL's, endpoints, etc... Basically, you are saying the client is no longer going to be used.

[rmannibucau]

Yes, basically all the volatile (threadlocal) states could be closed here and the longer time cache (wsld etc) can be put in the bus or equivalent (by default) and therefore keep the client.close fast and the overall client efficient. Issue with these new API is that you can't write a portable app anymore. This is my main concern.
Ensuring the client set and reset the state properly is surely saner for all consumers (native cxf, jaxws, jaxrs), no?

[fsgonz]

I am taking the issue from @facundovs and I was reviewing the fix introduced in CXF-7591 as @dkulp mentioned. At first I thought that that may provide the users with a way to clear the context using: getResponseContext().clear() (as you said) , but I find that in every case where responseContext.put is used the clear method is not overridden so the when I invoke client.getResponseContext().clear(), the map associated to the thread key is removed but not the the entry with the thread.
In the case where a ThreadPool is used, a WeakHashMap will make no difference.
Shouldn't the clear method of the hashmap always be overridden? For example here:

cxf/core/src/main/java/org/apache/cxf/endpoint/ClientImpl.java

Line 338 in 1396f04

Map<String, Object> resp = new HashMap<>();