search

TakahikoKawasaki / nv-websocket-client

High-quality WebSocket client implementation in Java.
Apache License 2.0
2.03k stars 292 forks source link

Should we clear mCopiedListeners when invoking ListenerManager.clearListeners? #180

Closed wsxyeah closed 5 years ago

wsxyeah commented 5 years ago

Even I called ListenerManager.clearListeners, ListenerManager.mCopiedListeners still keeps strong references of my listener, which may cause memory leak.

Should we clear mCopiedListeners at that time?

TakahikoKawasaki commented 5 years ago

I'm sorry for the implementation and thank you for your finding the issue. I released nv-websocket-client 2.7 that includes the fix for the issue just now. I hope it can solve the issue.

wsxyeah commented 5 years ago

Thank you your reply.

But I got one question about the synchronization: when there is any iterator of mCopiedListeners is in use(just like callOnFrame method), in the meantime we call mCopiedListeners .clear(), this may cause ConcurrentModificationException, so it's not quite safe, right?

eku commented 5 years ago

@TakahikoKawasaki how about using collections from java.util.concurrent, i.e. CopyOnWriteArrayList in stead of rolling your own synchronisation?

TakahikoKawasaki commented 5 years ago

Thank you for pointing it out. I'm thinking of fixing the code like below.

public void clearListeners()
{
    synchronized (mListeners)
    {
        if (mListeners.size() == 0)
        {
            return;
        }

        mListeners.clear();
        mCopiedListeners = null;
        mSyncNeeded = true;
    }
}

Do you have any concerns about this?

BTW, the reason nv-websocket-client does not use java.util.concurrent is mainly because I was not familiar with the package when I started to implement nv-websocket-client. The reason I don't replace synchronization operations in nv-websocket-client with java.util.concurrent is that I don't have time to maintain this library. I update this library only when a critical issue is reported and I estimate it can be easily fixed. Sorry for this inconvenience.

eku commented 5 years ago

@TakahikoKawasaki If you don't mind I'll supply a PR.

TakahikoKawasaki commented 5 years ago

@eku Thank you, but a problem is that I cannot spare enough time for review if a PR is big.

TakahikoKawasaki commented 5 years ago

Released nv-websocket-client 2.8 which includes the fix I suggested above.