[NEW] theoretical / experimental take on mongo-backend sessions #13424
Conversation
as discussed in RocketChat#10714 excluding package-lock, avoiding to bump dependencies I didn't mean to upgrade in the first place, ... pending further discussions, especially regarding configuration / environment variables we'ld want to use initializing mongodb sessions store
|
|
There was a problem hiding this comment.
Thanks @faust64 .. This is something we need for some time now but haven't time to look at.
Per CI tests seems your are not correct though:
Exception in callback of async function: TypeError: Cannot read property 'Store' of undefined
I'll need to give it a try to see what is going on.
I added some questions inplace, hope you don't mind answering.. thanks =)
…ss-session and stuff
and sure, I'm an idiot
|
Not sure what's next, @sampaiodiego , is everything OK? is it normal the build-and-test/hold jobs is still pending? |
|
thanks again @faust64 .. yes, the hold job stays pending, that's normal. I'll take a look again on this PR later this week. |
There was a problem hiding this comment.
Getting this error when trying to run it locally:
I20190304-11:36:15.406(-3)? Exception in callback of async function: TypeError: require(...) is not a function
I20190304-11:36:15.406(-3)? at index.js (packages/rocketchat:grant/server/index.js:24:24)
I20190304-11:36:15.407(-3)? at fileEvaluate (packages/modules-runtime.js:336:7)
I20190304-11:36:15.407(-3)? at Module.require (packages/modules-runtime.js:238:14)
I20190304-11:36:15.407(-3)? at require (packages/modules-runtime.js:258:21)
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/packages/rocketchat_grant.js:806:15
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/packages/rocketchat_grant.js:811:3
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/boot.js:411:36
I20190304-11:36:15.407(-3)? at Array.forEach (<anonymous>)
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/boot.js:220:19
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/boot.js:471:5
I20190304-11:36:15.407(-3)? at Function.run (/Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/profile.js:510:12)
I20190304-11:36:15.407(-3)? at /Users/rodrigonascimento/Projects/Rocket.Chat/.meteor/local/build/programs/server/boot.js:470:11
| storeAddress += '&connectTimeoutMS=1200'; | ||
| } | ||
| } | ||
| const SessionStorage = require('connect-mongodb-session')(session); |
There was a problem hiding this comment.
Please use import rather than require
| const SessionStorage = require('connect-mongodb-session')(session); | ||
| const store = new SessionStorage({ | ||
| collection: process.env.SESSION_COLLECTION || 'rocketchat_grant_sessions', | ||
| databaseName: process.env.SESSION_DATABASE || 'rocketchat', |
There was a problem hiding this comment.
Would this force the rocketchat database instead of getting from connection URI?
|
@faust64 ping on this, please accept the CLA and update to latest master. This PR might have the added side effect of not requiring us to use session/server affinity in HA setups since session data would then be shared, rather than server-local. I'm personally very much interested in seeing this make it into rocket.chat. |
|
@rodrigok @engelgabriel @faust64 @sampaiodiego |
|
@faust64 Can you sign the CLA? thanks |
|
@filipef101 it seems that he already signed the CLA repeatedly, but the CLA site is broken as heck. If someone else wants to pick this up and apply the requested changes I'm pretty sure faust is good with that as he has no further interest in contributing to Rocket.Chat (as per an email from him yesterday) due to arduous contributing requirements and concerns relating to scalability. |
|
The MemoryStore warning regarding memory leak only really applies if you enable our GraphQL experimental feature. Normal operations of the server are not affected, as we don't use ExpressJS for any other APIs. Having said that, we are planning to remove the GraphQL implementation from the main codebase and leave it to be added later as an auxiliary Apolo base project. |
|
@engelgabriel That's weird, because Rocket.Chat 1) doesn't have a shared session store, making HA really annoying and 2) has had memory leaks even without GraphQL enabled in my usage. |
|
See #14959 |
|
@infowolfe the GraphQL disable option was designed in a way that it does not really turn off GraphQL, it just makes the reply a 404. |
|
And it is not correct that Rocket.Chat does not have a shared session store, Meteor uses MongoDB to store the session authentication token. |
|
If that's the case, then why is it that I need to use session affinity in my load balancer in order for rocket.chat to not break completely? |
|
@infowolfe this only happens if you are not using WebSockets for the real-time data. Please see the link below for reference. |
Eventually, would address #10714
Didn't look at the docs yet, although there would be stuff to update.
This PR is mostly sent as a theoretical take on moving rocket's in-memory sessions storage into mongodb.
I did not test this code!
Excluding package-lock, avoiding to bump dependencies I didn't mean to upgrade in the first place, ... pending further discussions, especially regarding configuration / environment variables we'ld want to use initializing mongodb sessions store