Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't create sessions for update checks, update installs, and library downloads #604

Closed
sizzlemctwizzle opened this issue Mar 19, 2015 · 4 comments
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment.

Comments

@sizzlemctwizzle
Copy link
Member

Right now our production database is relatively small, with the exception of session storage. Express Mongo creates a session for anyone who accesses OUJS. This is problem for us because we are pounded with requests by GM and TM for update checks, update installs, and library downloads, which means a session is created for every instance of GM or TM that has a OUJS script installed. It doesn't matter if these users never login, register, or even visit the site, they all have sessions created for them. The solution to this problem is to find a way to bypass the session logic for /src/ paths so that session are not created.

This issue is blocking because there were 900,000+ session records when I cleared out the session data today, and within half an hour there were already +2,000. We need to keep session storage as small as possible for clear scale-ability issues. No new non-bugfix PRs will be merged until this issue is resolved. The site can't move forward if the database is exploding.

@sizzlemctwizzle sizzlemctwizzle added expedite Immediate and on the front burner. BLOCKING Concentrate on bug fixes primarily. CODE Some other Code related issue and it should clearly describe what it is affecting in a comment. labels Mar 19, 2015
@Martii
Copy link
Member

Martii commented Mar 20, 2015

Optimization wise, #285 will affect this as well with inclusion of pegjs as the object will become much larger... so that is a consideration too. This can be a reason why I mentioned at one point we should store the raw metadata and serialize it to an object on the fly... although I haven't done any real comparisons just yet.

@sizzlemctwizzle sizzlemctwizzle self-assigned this Mar 20, 2015
@sizzlemctwizzle
Copy link
Member Author

I'm working on a patch right now that should help until we can find something better. Should be finished in about an hour or so.

sizzlemctwizzle added a commit that referenced this issue Mar 20, 2015
… meta requests by intercepting them before the session middleware is used.
@sizzlemctwizzle
Copy link
Member Author

That PR should work, but it definitely isn't the smoothest way to solve the problem.

Edit: Btw, there are currently 24,000+ session documents atm.

sizzlemctwizzle added a commit that referenced this issue Mar 20, 2015
@sizzlemctwizzle sizzlemctwizzle removed BLOCKING Concentrate on bug fixes primarily. expedite Immediate and on the front burner. labels Mar 20, 2015
@sizzlemctwizzle
Copy link
Member Author

Well my patch seems to have slowed things a bit. It would be nice if we could only create session documents for users when/if they log in. Creating a document for every browser that accesses the site seems crazy. It also seems like a vulnerability.

Martii added a commit to Martii/OpenUserJS.org that referenced this issue Oct 25, 2017
* When logging out destroy the current session not only in the User model but also the session store
* Currently `maxAge` is set to expire at browser session end... client side cookie goes away at browser private data clear but sessionId in the store sticks around for quite some time. Logging out means destroy it and login again later.
* Leaving the old `delete` in for extra cautiousness... not really needed imho as it throws an error outside of it after `destroy()`

Related to OpenUserJS#604
Martii added a commit that referenced this issue Oct 25, 2017
* When logging out destroy the current session not only in the User model but also the session store
* Currently `maxAge` is set to expire at browser session end... client side cookie goes away at browser private data clear but sessionId in the store sticks around for quite some time. Logging out means destroy it and login again later.
* Leaving the old `delete` in for extra cautiousness... not really needed imho as it throws an error outside of it after `destroy()`

Related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Oct 26, 2017
* This can be tweaked to slightly larger but the default of two weeks server side is too much. Sync both client access and server expiration to this value.
* Going to bump all stored sessions in a few moments

Related to OpenUserJS#604
Martii added a commit that referenced this issue Oct 26, 2017
* This can be tweaked to slightly larger but the default of two weeks server side is too much. Sync both client access and server expiration to this value.
* Going to bump all stored sessions in a few moments

Related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Oct 26, 2017
Martii added a commit that referenced this issue Oct 26, 2017
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Nov 9, 2017
* Alternative is "Remember me" option although I think I'd rather have a "Forget me" option by default.

Post OpenUserJS#1202 and related to OpenUserJS#604
Martii added a commit that referenced this issue Nov 9, 2017
* Alternative is "Remember me" option although I think I'd rather have a "Forget me" option by default.

Post #1202 and related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Nov 9, 2017
* Alternative is "Remember me" option although I think I'd rather have a "Forget me" option by default.

Post OpenUserJS#1202 and related to OpenUserJS#604
Martii added a commit that referenced this issue Nov 9, 2017
* Alternative is "Remember me" option although I think I'd rather have a "Forget me" option by default.

Post #1202 and related to #604

Auto-merge
@Martii Martii mentioned this issue Jun 30, 2018
Martii added a commit that referenced this issue Jun 30, 2018
* The cost of `interval` mode is too high plus it also has a similar, yet muffled, issue as `native`. Each session gets it's own server side *(OUJS)* timer and that can be really CPU intensive if too many users pop on board at the same time.
* MongoDB remote is not cleaning up expired sessions until many, many, many hours later... this may subside at a later date but something we need to live with at the moment until the main DB is migrated to a newer version *(probably one minor semver at a time)*. \*hint hint\* @sizzlemctwizzle
* Reoutput the `originalMaxAge` as a label.
* New dep extension *(plugin as they call it)* that extends *moment*
* Max extension age is n *(currently 6)* times 3... may change these values after some more consultation

Post #1471 ... related to #604 

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 1, 2018
Martii added a commit that referenced this issue Jul 1, 2018
Post #1446 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 2, 2018
* Should be functionally equivalent to the inline but using current structures e.g. match project code style placements. Needed for next stage.
* Added one override on `parseDateProperty` since `date` sometimes gets written to a flat string object instead of `Date`.

Post OpenUserJS#1446 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 2, 2018
* Should be functionally equivalent to the inline but using current structures e.g. match project code style placements. Needed for next stage.
* Added one override on `parseDateProperty` since `date` sometimes gets written to a flat string object instead of `Date`.

NOTE: These are no longer sorted as that's coming up in the future... although may reinstate if lack of time

Post #1446 ... related to #604
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 2, 2018
* Now this is giving predictable results so far.

Refs:
* https://github.com/jdesboeufs/connect-mongo/blob/9f86f90/README.md#session-expiration
* https://github.com/jdesboeufs/connect-mongo/blob/9f86f90/README.md#remove-expired-sessions

NOTES:
These versions of the docs are ambiguous. The TTL is how often it reaches out to the DB... if it finds an expired session **or** a session cookie it will then remove it. So the inversion of the logic should fix this as it checks every 10ish minutes for expiry as well as session cookies and destroys them. Had a light bulb flip on with `interval` being doc'd at 10 minutes so I thought to try TTL at that... and voila. :)

This theoretically should solve OpenUserJS#604 by sticking to this methodology *(until MongoDB changes something heh)*

When available will do some code condensing to reuse some more code if applicable.

Post OpenUserJS#1471 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 2, 2018
* Now this is giving predictable results so far.

Refs:
* https://github.com/jdesboeufs/connect-mongo/blob/9f86f90/README.md#session-expiration
* https://github.com/jdesboeufs/connect-mongo/blob/9f86f90/README.md#remove-expired-sessions

NOTES:
These versions of the docs are ambiguous. The TTL is how often it reaches out to the DB... if it finds an expired session **or** a session cookie it will then remove it. So the inversion of the logic should fix this as it checks every 10ish minutes for expiry as well as session cookies and destroys them. Had a light bulb flip on with `interval` being doc'd at 10 minutes so I thought to try TTL at that... and voila. :)

This theoretically should solve #604 by sticking to this methodology *(until MongoDB changes something heh)*

When available will do some code condensing to reuse some more code if applicable.

Post #1471 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 2, 2018
* For those strays that don't get destroyed.

Post OpenUserJS#1471 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 2, 2018
* For those strays that don't get destroyed.

Post #1471 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 6, 2018
* Adjust values to shorter, preferred, intervals
* Additional sanity check for idle session from browser blocked and query accept redirect with JavaScript disabled.
* Restore OpenUserJS#1448 *(not found in stderr and new users are okay)*... ends mitigation from OpenUserJS#1449 ... most likely a critical DB failure with mongolabs that should be trapped and handled
* Move destruction to stderr with debug mode only... Applies to OpenUserJS#430

NOTE:
* Unit specific tests... if changed must keep in `m` and `h`. `nominal` replacement and `maximum` addition are in `h`... the rest are `m`

Post OpenUserJS#1471 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 6, 2018
* Adjust values to shorter, preferred, intervals
* Additional sanity check for idle session from browser blocked and query accept redirect with JavaScript disabled.
* Restore #1448 *(not found in stderr and new users are okay)*... ends mitigation from #1449 ... most likely a critical DB failure with mongolabs that should be trapped and handled
* Move destruction to stderr with debug mode only... Applies to #430

NOTE:
* Unit specific tests... if changed must keep in `m` and `h`. `nominal` replacement and `maximum` addition are in `h`... the rest are `m`

Post #1471 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 6, 2018
NOTE:
* I'm not entirely sure if modelQuery can be reused here but will reexamine at each stage

Post OpenUserJS#1471 ... related to OpenUserJS#604
@Martii Martii mentioned this issue Jul 6, 2018
Martii added a commit that referenced this issue Jul 6, 2018
NOTE:
* I'm not entirely sure if modelQuery can be reused here but will reexamine at each stage

Post #1471 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2018
* Iterate one session document at a time ... useful when we get lots of users auth'ed
* Rename function to more like MongoDB function.
* Very, very, very simple search algorithm... this may be expanded in later stages if applicable and probably will not be matching query feature set in MongoDB/*mongoose*
* Reinvert sort order... more like on a User page listing comments instead of Discussion comments
* Fix view to a list group a little more
* Remove some unnecessary items from view paste
* Remove an orphaned identifier

NOTE:
* More changes probably ... more error checking too

Post OpenUserJS#1446 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 7, 2018
* Iterate one session document at a time ... useful when we get lots of users auth'ed
* Rename function to more like MongoDB function.
* Very, very, very simple search algorithm... this may be expanded in later stages if applicable and probably will not be matching query feature set in MongoDB/*mongoose*
* Reinvert sort order... more like on a User page listing comments instead of Discussion comments
* Fix view to a list group a little more
* Remove some unnecessary items from view paste
* Remove an orphaned identifier

NOTE:
* More changes probably ... more error checking too

Post #1446 ... related to #604

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2018
@Martii Martii mentioned this issue Jul 7, 2018
Martii added a commit that referenced this issue Jul 7, 2018
Post #1446 ... related to #604 

Auto-merge
Martii added a commit to Martii/OpenUserJS.org that referenced this issue Jul 7, 2018
* Scoot a bunch of code around for reuse

Post OpenUserJS#1446 ... related to OpenUserJS#604
Martii added a commit that referenced this issue Jul 7, 2018
* Scoot a bunch of code around for reuse

Post #1446 ... related to #604

Auto-merge
@OpenUserJS OpenUserJS locked as resolved and limited conversation to collaborators Apr 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CODE Some other Code related issue and it should clearly describe what it is affecting in a comment.
Development

No branches or pull requests

2 participants