search

mongodb-js / connect-mongodb-session

Lightweight MongoDB-backed session store for Connect and Express
Apache License 2.0
175 stars 35 forks source link

connect-mongo convergence #15

Open jdesboeufs opened 9 years ago

jdesboeufs commented 9 years ago

Hi,

I'm the maintainer of connect-mongo since the end of 2014. What could be the plan to merge? :)

vkarpov15 commented 9 years ago

Heya. Just been looking at the work you've done with connect-mongo, it's looking a lot better than it was back when I started this module in late 2014 :) Among my goals for starting this module where:

  1. Support replica sets
  2. 100% coverage
  3. Don't allow re-using existing connection pools
  4. Implement the minimum feature set necessary to get something running

Our modules still differ on the latter 3 points. I'm also hesitant about connect-mongo using babel to transpile for NodeJS - that seems unnecessary. Would you be interested in discussing these details?

FWIW I'm unlikely to move my code off of this module in the foreseeable future. I moved off of connect-mongo last year because connect-mongo was crashing my server every single time the replica set primary went down. connect-mongodb-session has been happily running along without a single server crash, so I don't have a good reason to take the time to switch.

jdesboeufs commented 9 years ago

I had the same issues than you in late 2014, that's why I asked to Casey to become maintainer.

100% coverage: Ok! We are at 82% today for now, with many ugly tests.

Allowing existing connection pool re-use is important for me. Many of our users re-use their mongoose connection. Can you explain your position on that? Maybe I misunderstand something.

Implement minimal feature set: Ok! Some extra features will be dropped in the next version since they are too specific or not tested enough. Your advice could be useful to know which ones too keep.

I use babel to take benefit of class declaration syntax and arrow functions + promises. Rewrite in ES6 in experimental, and in progress.

vkarpov15 commented 9 years ago

My reasoning for not supporting shared connection pools is 1) too many mongoose issues related to old versions of connect-mongo, 2) easier to reason about performance, 3) cleaner API.

Re: point 2, MongoDB can only handle one operation per connection at a time - slow running operations will block operations coming in on the same pool. Since the mongodb node driver does a strict round-robin over connections in the pool, your session operations might get backed up behind slow queries, etc. Putting the express session store behind its own connection pool makes performance more consistent.

Re: point 3, it's cleaner to just throw a URI at the session store and forget about it IMO. Means I don't have to worry about mongodb driver version compatibility or users doing something weird with the connection. For instance, what happens if a user closes the shared connection or switches the db underneath? There are tradeoffs either way of course, shared connection gives you more customization options, but also more ways to shoot yourself in the foot (which I've done more than a few times with managing connection state).