Fixes bug: cleanup deletes all sessions

multimeric / koa-pg-session

A model implementation of sessions for koa using postgres as the backend

10 stars 7 forks source link

Fixes bug: cleanup deletes all sessions #9

Closed JonahBraun closed 8 years ago

JonahBraun commented 8 years ago

I came here to fix the same bug as @dizlexik in #8 . Opening anyway (rebasing on his) as I recommend simplifying and just using Postgres' now() function.

Relevant docs:

Date.now() outputs milliseconds.
to_timestamp() expects seconds.
now() outputs the current date and time.

JonahBraun commented 8 years ago

Forgot to add steps to reproduce the bug:

Change cleanup time to one minute
Start a session
Wait one minute
Observe all sessions are deleted

dizlexik commented 8 years ago

Hey @JonahBraun, I actually had the same idea when I applied my fix but decided against using now() since the insertValueSql and updateValueSql are called with values derived from Date.now() and this could cause issues if the app server and database server's times are not in sync.

Ultimately it probably is a better idea to use the database server's time, as you've done here, but not without also updating insertValueSql and updateValueSql. Would you agree?

JonahBraun commented 8 years ago

That's a good point, I agree. I didn't consider the possibility someone might be using two different servers where the clocks are significantly different, but it could happen!

Using Postgres' interval would work to set the expiry. As it stands right now though, your pull request is better.

JonahBraun commented 8 years ago

Closing this in favor of @dizlexik's #8. @TMiguelT feel free to ping me if you need additional information this.