Non transactional session - Sequelize findOrCreate

[imranbohoran]

SequelizeStore.prototype.set currently uses the sequelize findOrCreate that selects and inserts (if necessary) in a transaction.
For the purpose of sequelize store set, this is probably not required as
updating the same session concurrently in more than one request might not be
applicable.
The reason for this PR is the observation of high memory usage and eventually crashing the DB
under postgres when findOrCreate is used. The underlying reason for this is that sequelize uses temp functions setup transactions and trace any failures. Postgres keeps a cache of temp functions and under high rate of creating sessions (by users hitting a page that requires session creation), postgress can't cleanup the cache and as a result falls over once all of the swap is utilised.

This change is to use the more performant non-transactional findCreateFind function as per a discussion with the sequelize community in their slack channel.

This pull request questions if transactions are really required for the SequelizeStore.prototype.set and if not proposing to use the findCreateFind.

[mweibel]

please remove that and add it to your global gitignore file.

[imranbohoran]

Done.

[mweibel]

Thanks for your pull request. I'll need to get myself updated on this as I don't know about the difference between those two APIs.

For the purpose of sequelize store set, this is probably not required as
updating the same session concurrently in more than one request might not be
applicable.

A browser can do two requests to the same server in a concurrent way, therefore the session might get updated. On the other hand, actually changing the session in a meaningful way, concurrently, is probably not happening usually. Do you happen to run that on production already?
I'm happy to merge that and maybe release a beta release, to see if there are any issues with it.
There's however a test failing. Do you know why?

[imranbohoran]

Thanks for getting back. We are not using it in production, and yes I would agree on the observation on concurrent session updates not being usual.

I'm not entirely sure why the test is failing. Doesn't seem to be failing for me on my laptop at least (Works on my machine ™) I'll dig around a bit more to see why it could be failing.

[imranbohoran]

@mweibel Looks like the when running in travis, TestSessions is not getting created when the test runs. I've added some console out and it shows a SequelizeDatabaseError: SQLITE_ERROR: no such table: TestSessions error. Have you seen this before?

[mweibel]

hmm, not that I know of. Will re-run an old build to see if something has changed.

[mweibel]

Current master works fine.. weird. Why should table creation have anything to do with the .set call change? ;)

[imranbohoran]

Yes, looks like the switch to findCreateFind is somehow causing it. But a table not being available is the last thing I was expecting. The fat that I can't reproduce it locally is a bit frustrating :(

[imranbohoran]

@mweibel So it looks like we are hitting a race-condition in the tests. The store.sync() in the before function (of the test that fails), seem to be completing after the test has begun. Hence the following sequence of log lines when tests fail.
Executing (default): SELECT sid, userId, expires, data, createdAt, updatedAtFROMTestSessionsASTestSessionWHERETestSession.sid= '1234a'; Executing (default): CREATE TABLE IF NOT EXISTSTestSessions (sidVARCHAR(255) PRIMARY KEY,userIdVARCHAR(255),expiresDATETIME,dataVARCHAR(50000),createdAtDATETIME NOT NULL,updatedAt DATETIME NOT NULL); Error : SequelizeDatabaseError: SQLITE_ERROR: no such table: TestSessions 1) should extend defaults when extendDefaultFields is set

The select has fired before the create statement (i.e. tests have started before store.sync) is completed. Perhaps in the when findOrCreate was used, due to the overhead of creating the transactions, temporary functions in sequelize, we didn't hit the race condition as it probably gave enough time for the store.sync() to complete. Since this PR uses the more performant findCreateFind function from sequelize, it opens up the test for the race condition. Like I said, I can't reproduce it locally. But that's the nature of race-conditions anyways.

The tests in the PR are all green after I added a console.log following the promise completion and returned. It turns out just returning the store.sync() is sufficient.

Can you have a look and let me know your thoughts and if you're happy to merge it. The commits are a bit messy, so I need to clean it up before its merged to master though.

[imranbohoran]

@mweibel Based on my note above, I'm not convinced that the return is sufficient. After all what we want is the test to not run until the store.sync() has completed. Just doing a return or passing a function to then is still leaving it to chance. To be more confident that the tests only run once the store.sync has completed successfully, I've changed the test should extend defaults when extendDefaultFields is set to be a callback on the store.sync (Yay! promises). Let me know what you think, and is possibly a safer approach for other tests as well.

[mweibel]

Great, thanks for your effort!
I'm gonna merge this and will publish version 4.1.0-beta.1 afterwards.

[mweibel]

Published version 4.1.0-beta.1 on npm and tagged the release, please let me know if there are any issues :)

As you've invested time into doing this PR and did so very well, I'll make you a collaborator. If you don't want, just decline the invite ;)
Thanks again!