search

srinathgs / mysqlstore

Gorilla's Session Store Implementation for MySQL
http://godoc.org/github.com/srinathgs/mysqlstore
MIT License
44 stars 28 forks source link

Add test; maintain long-term connections. #1

Closed fiatflux closed 10 years ago

fiatflux commented 10 years ago

Added a test based on https://github.com/boj/redistore, and arranged for long-term connection and statement life.

srinathgs commented 10 years ago

Hi Gregor,

Thanks for contribution. The tests files shall be merged.

Got a few questions though.

1) Why should the timezone be UTC?

2) I could see that Stmt is threadsafe. Though I'm surprised to see that there was no drastic change in the number of connections to the Database. I wrote a small web server (with the modified version) and ran ab on it to test. Anyway, the changes you have made are looking good, and shall be included.

3) My connection DSN had ?parseTime=true&loc=Local. Because of this, all the sessions were expired. I got this error 2013/11/05 11:59:33 Session expired on 2013-11-05 07:29:32 +0530 IST, but it is 2013-11-05 06:29:33.527863562 +0000 UTC now..May be we can use URL.Parse from net/url and use the timezone specified by loc parameter as mentioned here. Most of the times, webserver and DB server are kept in the same timezone but not necessarily UTC. Probably we can do something with Time.In function from package time

Once we resolve this third issue, we are good to merge.

Thanks

fiatflux commented 10 years ago

Laundry list of comments....

2) Rumor on the street is that the DB objects are supposed to be long-lived. Behind the scenes they dynamically create connections. https://github.com/VividCortex/go-database-sql-tutorial

4) I accidentally left an experimental bit in the create table statement.

5) There are some cleanups I want to do in the test.

6) It's probably worth mentioning that the API has changed in this commit, and the store now requires a Close() when its use is over. In most cases I don't think users will notice a difference since the store will generally live until program termination; more sophisticated failover and similar mechanisms may run into resource problems.

7) I'm thinking about asking Gorilla to include a Store interface that includes Close() and Delete().

fiatflux commented 10 years ago

Not sure what I was thinking with the timezone issues... users can deal with their own timezone issues in endpoint description. Later it might be nice to encapsulate the driver-specific endpoint, but that's for later. This PR is now actually a PR :-P

srinathgs commented 10 years ago

Thanks for writing the tests and making the changes. :+1: I have merged your pull request. Moreover, it might be a good idea to ask gorilla sessions to add store.Delete to their interface. mysqlstore works independent of it. If you can update README for this, it will be great. But it may also be noted that for gorilla session authors, it might call for an API change which they might resist. Anyway, we may ask them.

Thanks.