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

Memory leak in identify if opening a stream fails. #419

Closed
Stebalien opened this Issue Sep 10, 2018 · 1 comment

Comments

1 participant
@Stebalien
Copy link
Contributor

Stebalien commented Sep 10, 2018

We need to remove the connection from the connection tracker in both of these cases:

s, err := c.NewStream()
if err != nil {
log.Debugf("error opening initial stream for %s: %s", ID, err)
log.Event(context.TODO(), "IdentifyOpenFailed", c.RemotePeer())
c.Close()
return
}
s.SetProtocol(ID)
// ok give the response to our handler.
if err := msmux.SelectProtoOrFail(ID, s); err != nil {
log.Event(context.TODO(), "IdentifyOpenFailed", c.RemotePeer(), logging.Metadata{"error": err})
s.Reset()
return
}

@Stebalien

This comment has been minimized.

Copy link
Contributor

Stebalien commented Sep 10, 2018

That is, from here:

ch := make(chan struct{})
ids.currid[c] = ch
ids.currmu.Unlock()

See ipfs/go-ipfs#5412

@Stebalien Stebalien added the bug label Sep 10, 2018

Stebalien added a commit that referenced this issue Sep 10, 2018

always remove connection from identify service map
fixes #419

Also call FullClose in a goroutine; no need to block this.

(not happy with that but I'm starting to think we need to rethink stream
closing, again...)

@wafflebot wafflebot bot added the in progress label Sep 10, 2018

@wafflebot wafflebot bot removed the in progress label Sep 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment