Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coverity - resource leak in client's ProxyManager #11751

Closed
kwart opened this issue Nov 9, 2017 · 3 comments
Closed

Coverity - resource leak in client's ProxyManager #11751

kwart opened this issue Nov 9, 2017 · 3 comments

Comments

@kwart
Copy link
Contributor

@kwart kwart commented Nov 9, 2017

The method ProxyManager.getOrCreateProxy() creates a ClientProxy even if it's not used - i.e. when it already have a ClientProxyFuture instance in the proxies Map:

final ClientProxy clientProxy = createClientProxy(id, factory);
proxyFuture = new ClientProxyFuture();
final ClientProxyFuture current = proxies.putIfAbsent(ns, proxyFuture);
if (current != null) {
    return current.get();
}

https://scan4.coverity.com/reports.htm#v32322/p13030/fileInstanceId=29225745&defectInstanceId=5757191&mergedDefectId=202188&eventId=5757191-18

@sancar
Copy link
Member

@sancar sancar commented Nov 10, 2017

@kwart
This design in intentional.
Extra proxy creation only happen when there is concurrent calls to this method.
In other cases, the proxies map is checked a few lines before as follows:

ClientProxyFuture proxyFuture = proxies.get(ns);
        if (proxyFuture != null) {
            return proxyFuture.get();
        }
@sancar sancar closed this Nov 10, 2017
@sancar sancar reopened this Nov 10, 2017
@kwart
Copy link
Contributor Author

@kwart kwart commented Nov 10, 2017

Is there a reason why not to move the clientProxy = createClientProxy(id, factory); few lines down to only happen if current==null?

@sancar
Copy link
Member

@sancar sancar commented Nov 10, 2017

I took a second look. Yes, you are right. we can move down client proxy creation.
I was assuming that clientProxy is used when creating ProxyFuture. And we can not move the proxyFuture .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.