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

Add refcounting to avoid close connection by mistake #77051

Closed
wants to merge 5 commits into from

Conversation

kkewwei
Copy link
Contributor

@kkewwei kkewwei commented Aug 31, 2021

The connection will be closed by node-left task, at the same time the connection maybe have been reused by join request, This commit import refcounting to solve the problem.

Relates #67873

@elasticsearchmachine elasticsearchmachine added v8.0.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Aug 31, 2021
@kkewwei kkewwei changed the title fix close connection by mistake Add refcounting to avoid close connection by mistake Aug 31, 2021
ConnectedNodeRefCounter connectedNodeRefCounter = connectedNodes.get(node);
if (connectedNodeRefCounter != null) {
if (connectedNodeRefCounter.isDeleting) {
connectedNodeRefCounter.incRef();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this will leak: we might call connectToNode multiple times while the isDeleting flag is set (e.g. handling multiple joins from the same node, or else an old PeerFinder might still be active). We need to make sure that every connectToNode call explicitly releases any references that it obtained.

@kkewwei
Copy link
Contributor Author

kkewwei commented Sep 2, 2021

I use reuse instead of refcounting to avoid excess count. When to close the connection depends on the reuse situation at that time.

It seems difficult to work out exactly when to release the ActionListener<Releasable> in TransportService#connectToNode.

@DaveCTurner
Copy link
Contributor

Difficult, yes, but unfortunately it seems necessary too. I spent some time on this yesterday and although I'm not ready to open a PR you can see my work here: master...DaveCTurner:2021-09-01-releasable-connections. I think this gets the lifecycles about right, and I love how much it simplifies the NodeConnectionsService, but there's a few outstanding test issues still to resolve and I need to think about it some more with a fresh mind too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants