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

Change default session store to cookie store when in production mode #1648

Merged
merged 4 commits into from
Oct 3, 2022

Conversation

lfdebrux
Copy link
Member

@lfdebrux lfdebrux commented Oct 3, 2022

Change the default configuration of the kit so that when the kit is run in production (NODE_ENV=production) the cookie session store is used instead of the memory session store. The impact of this is that session data will default to be preserved between server restarts, and the user will not see a warning from express-session in their server logs every server restart.

This PR still defaults to using the memory session store when running the kit locally, and the configuration can still be overridden.

We want to be able to change environment variables in the command line
tool before starting the kit implementation, but that means we need to
make sure that no imports/requires are done until the last moment.
Change the default configuration of the kit so that when the kit is run
in production (`NODE_ENV=production`) the cookie session store is used
instead of the memory session store. The impact of this is that session
data will default to be preserved between server restarts, and the user
will not see a warning from `express-session` in their server logs every
server restart.

This commit keeps memory store as the default when running the kit
locally, and the configuration can still be overridden.
@lfdebrux lfdebrux force-pushed the ldeb-use-session-store-when-hosted branch from b63849e to 19146e3 Compare October 3, 2022 11:48
Copy link
Contributor

@BenSurgisonGDS BenSurgisonGDS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lfdebrux lfdebrux merged commit 475032d into v13 Oct 3, 2022
@lfdebrux lfdebrux deleted the ldeb-use-session-store-when-hosted branch October 3, 2022 12:21
@joelanman
Copy link
Contributor

I think there's a potential issue with this - cookies have a max size of about 4k, and will silently fail if you try to store more

@lfdebrux
Copy link
Member Author

lfdebrux commented Oct 4, 2022

I think there's a potential issue with this - cookies have a max size of about 4k, and will silently fail if you try to store more

Hmm, okay, do we have user reports of this issue affecting them?

@joelanman
Copy link
Contributor

no, but I'm not aware of anyone using this option - I dont think its even documented, and its the reason we didnt make it default

@joelanman
Copy link
Contributor

discussed with @lfdebrux :

  • would be useful to persist session, both locally as a user is making a prototype (and triggering restarts), and remotely where a hosting service may restart, especially on a free tier.
  • cookie store failing over 4k is a problem - a text field could easily consume 4k
  • file store is a possibility but again may fail on some hosting services
  • if we dont recommend cookie store for this reason should we just remove it entirely?
  • we don't want users to worry about the current memory store warning when running remotely - it will not affect them. If we stick with Memory store maybe we should squash that warning if possible

@lfdebrux to investigate multi cookie store which may give us the persistence without the 4k limit

@edwardhorsford
Copy link
Contributor

edwardhorsford commented Oct 5, 2022

Our prototype stores a lot in session memory.

I'm not entirely sure how to tell how much we're using, but I saved it off as json in a file. The raw file is 23 megabytes. Compressed is 2.4mb.

Even for a small service, 4k is a very low limit - you could easily breach it.


Aside - does this get sent to and from the client with each request? would that slow things down?

@lfdebrux lfdebrux mentioned this pull request Oct 5, 2022
3 tasks
@lfdebrux
Copy link
Member Author

lfdebrux commented Oct 5, 2022

Our prototype stores a lot in session memory.

I'm not entirely sure how to tell how much we're using, but I saved it off as json in a file. The raw file is 23 megabytes. Compressed is 2.4mb.

Thanks for the info, that is useful. Just to check, is that from multiple sessions with multiple users? Also, just out of interest, what is the composition of the session data? How many of the questions are checkboxes/radios, and how many are free-form text fields?

Even for a small service, 4k is a very low limit - you could easily breach it.

I think it depends on the kind of service and the kind of questions asked, but agree it is a little stingy. With splitting cookies we could get at most 80 KiB, which is a bit more generous, but perhaps still not enough?

Aside - does this get sent to and from the client with each request? would that slow things down?

Yes, that is a consideration.

@edwardhorsford
Copy link
Contributor

@lfdebrux this is data pre-loaded with session defaults.

My service is a casework service, so we have seed records for a few thousand trainees. Within each trainee is a bunch of simulated data. We've also got a few sizeable datasets for use in autocompletes and in routes. One of them is probably what makes us 20mb rather than 2mb. I actually moved it out of session data for this reason, but think I may have a bug and have re-loaded it somewhere.

@joelanman
Copy link
Contributor

joelanman commented Nov 9, 2022

to note, we changed to using files to store session #1658

This was referenced Nov 17, 2022
lfdebrux added a commit that referenced this pull request Nov 18, 2022
Remove #1648 from changelog we backed out those changes in #1658.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants