Fix "not master" error in mongo plugin #7743
Conversation
|
|
| func (c *mongoDBConnectionProducer) Close() error { | ||
| c.Lock() |
There was a problem hiding this comment.
Locking inside Close() always produces a deadlock, as all of the calling functions (from mongodb.go) have locked the mutex already.
There was a problem hiding this comment.
Close can be called externally which is why we need to lock.
There was a problem hiding this comment.
Alright, I see two possibilities for dealing with that then.
-
Unlocking before calling it, and then locking again once done. (downside being that the lock needs to be acquired twice, and that another process can interfere inbetween the locking and unlocking)
-
Not calling it at all, and just closing the session by referencing the mongoDBConnectionProducer in mongodb.go. I had a commit where I did exactly this, and it worked ok.
What do you prefer?
plugins/database/mongodb/mongodb.go
Outdated
| } | ||
| case strings.Contains(err.Error(), "not master"): | ||
| // Close connection and reconnect if connected node is not primary. | ||
| if err := m.Close(); err != nil { |
There was a problem hiding this comment.
I don't think we need to call Close here, getConnection should take care of closing the session. Do we also want to retry the query afterwards? If so, we might just combine the EOF case and this one into one (similar to what you're doing in RevokeUser.
There was a problem hiding this comment.
Does getConnection really close the session in this case though? What happens if the session is not nil already and the ping succeeds?
There was a problem hiding this comment.
As far as I can see getConnection calls Connection() in connection_producer.go which only closes the session if the session is not nil and the ping fails.
The ping won't fail though, as the node is still up (it's just not the primary). I don't see a way to check whether it's master with the ping command.
plugins/database/mongodb/mongodb.go
Outdated
| @@ -251,7 +279,7 @@ func (m *MongoDB) RevokeUser(ctx context.Context, statements dbplugin.Statements | |||
| err = session.DB(db).RemoveUser(username) | |||
| switch { | |||
| case err == nil, err == mgo.ErrNotFound: | |||
| case err == io.EOF, strings.Contains(err.Error(), "EOF"): | |||
| case err == io.EOF, strings.Contains(err.Error(), "EOF"), strings.Contains(err.Error(), "not master"): | |||
| if err := m.Close(); err != nil { | |||
There was a problem hiding this comment.
I'm surprised we've not bumped into a deadlock here, but we should remove this Close call.
There was a problem hiding this comment.
Agreed. I removed it and replcaed it with a different way of closing that doesn't lead to deadlock.
|
I can confirm that the current state of this PR fixes the bug. Let me know if you need any revisions to the code. Looking forward to getting this merged. |
|
Pinging @calvn What do you think of the changes? What would it take to get this merged? |
| if m.mongoDBConnectionProducer.session != nil { | ||
| m.mongoDBConnectionProducer.session.Close(); | ||
| } | ||
| m.mongoDBConnectionProducer.session = nil |
There was a problem hiding this comment.
We could move this into the conditional above so that we always nil out the session after we close it.
| return errwrap.Wrapf("error closing EOF'd mongo connection: {{err}}", err) | ||
| case err == io.EOF, strings.Contains(err.Error(), "EOF"), strings.Contains(err.Error(), "not master"): | ||
| if m.mongoDBConnectionProducer.session != nil { | ||
| m.mongoDBConnectionProducer.session.Close(); |
There was a problem hiding this comment.
Can we set the session to nil afterwards?
| // Close connection and reconnect if connected node is not primary. | ||
| if m.mongoDBConnectionProducer.session != nil { | ||
| m.mongoDBConnectionProducer.session.Close(); | ||
| } |
There was a problem hiding this comment.
Can we set the session to nil afterwards?
|
I believe this should be fixed by #8140, going to close for now. Please let me know if you think this is a mistake. |
Fixes #7742.
Related: #2973