Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Do not use global sessions #361
Conversation
wallyworld
added some commits
Jul 21, 2014
davecheney
reviewed
Jul 23, 2014
| +import "labix.org/v2/mgo" | ||
| + | ||
| +// CollectionFromName returns a named collection on the specified database, | ||
| +// initialised with a new session. Also returned is a close function which is |
davecheney
Jul 23, 2014
Contributor
is or must be called ? What happens if we fail. Should you use a finalizer to catch failures to call session.Close ?
wallyworld
Jul 23, 2014
Owner
In our test suites, if there are active sockets still around when the tests complete, the test/suite teardown fails with an error. So this catches failures to call close.
davecheney
reviewed
Jul 23, 2014
| @@ -181,6 +181,15 @@ func (s *MongoSuite) assertAddRemoveSet(c *gc.C, root *gitjujutesting.MgoInstanc | ||
| // two copies of root in the replica set | ||
| members = append(members, Member{Address: getAddr(root), Tags: initialTags}) | ||
| + // We allow for up to 2m per operation, since Add, Set, etc. call |
davecheney
Jul 23, 2014
Contributor
2m ? two meters, two million, two minutes ? Please don't abbreviate here
davecheney
reviewed
Jul 23, 2014
| @@ -80,7 +80,7 @@ func (s *stateSuite) TestClientNoNeedToPing(c *gc.C) { | ||
| c.Assert(err, gc.IsNil) | ||
| } | ||
| -func (s *stateSuite) TestAgentConnectionShutsDownWithNoPing(c *gc.C) { | ||
| +func (s *pingerSuite) TestAgentConnectionShutsDownWithNoPing(c *gc.C) { | ||
| s.PatchValue(apiserver.MaxClientPingInterval, time.Duration(0)) | ||
| st, _ := s.OpenAPIAsNewMachine(c) |
davecheney
reviewed
Jul 23, 2014
| + return | ||
| + } | ||
| + err = fmt.Errorf("%v", v) | ||
| + return |
davecheney
reviewed
Jul 23, 2014
| + statusesC = "statuses" | ||
| + stateServersC = "stateServers" | ||
| + openedPortsC = "openedPorts" | ||
| + // These collections are used by the mgo transaction runner. |
davecheney
reviewed
Jul 23, 2014
| +// If st has been authenticated by having it's database logged in, | ||
| +// a new mgo.Session is used. | ||
| +func (st *State) txnRunner() (_ jujutxn.Runner, closer func()) { | ||
| + closer = func() {} |
davecheney
Jul 23, 2014
Contributor
suggestion,
func emptycloser() {}
then return this rather than a function literal.
This may be important because any stack trace or printing of this value will return its name, not _anonfunc_00001 or something unhelpful.
|
LGTM. Some minor comments which you can ignore if they are unhelpful |
|
LGTM |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Jul 23, 2014
wallyworld commentedJul 23, 2014
Each time a mongo interaction is performed, the session is copied using session.Copy() to ensure a fresh socket is used if needed. The basis of the change is to remove the "global" mongo collections on state.State and refresh each time one is used. A similar approach is used with the transaction runner.
The changes essentially follow the mgo driver guidelines:
newSession := session.Copy()
defer newSession.Close()
During bootstrap, state.State is created using a session that has not had Login() called. For this case, the session is not copied as there are no stored credentials yet. An authenticated bool is added to state.State for this purpose.