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

session.touch is called on every request when resave: false #891

Closed
huntharo opened this issue May 14, 2022 · 14 comments
Closed

session.touch is called on every request when resave: false #891

huntharo opened this issue May 14, 2022 · 14 comments
Labels

Comments

@huntharo
Copy link

huntharo commented May 14, 2022

Description

[PR incoming with unit tests and implementation of option - default to no change from current behavior]

session.touch is called on every request when resave: false is set.

On in-memory stores such as connect-redis this has little impact other than additional traffic.

However, on disk-backed and/or globally replicated stores the costs can be significant, such as connect-dynamodb

Example Scenario with Costs - DynamoDB Store

DynamoDB Provisioned Capacity Pricing

image

  • RCU = Read Capacity Unit
  • WCU = Write Capacity Unit
  • 60,000 RPM site / 60 seconds / minute = 1000 RPS
  • Write on Every Request
    • 1,000 RPS requires
      • 1,000 RCU x $0.00013 = $0.13 / hour = $1,138 / year
      • 1,000 WCU x $0.00065 = $0.65 / hour = $5,964 / year
      • Total = $7,102 / year
  • Write on Update or when 75% of originalMaxAge Expired
    • Assuming 1% of requests update the session or reach 75% of originalMaxAge
    • 1,000 RPS requires
      • 1,000 RCU x $0.00013 = $0.13 / hour = $1,138 / year
      • 10 WCU x $0.00065 = $0.0065 / hour = $57 / year
      • Total = $1,195
      • Savings = 83% vs Write on Every Request

Best cases for cost reduction

  • Relatively longer sessions (e.g. minutes, hours, days, months, but not seconds)
  • Session values infrequently updated
huntharo added a commit to huntharo/session that referenced this issue May 14, 2022
@dougwilson
Copy link
Contributor

Hello, this is correct. Touching a session is the load of a session. Resave just controls saving the session data back to the store. Whenever a session is loaded from a store, it is touched. In order for a session not to be touched, it would be not loaded at all from the store. I hope that helps. As stated in the readme, the touch call signals that the session is active -- a request loading a session is session activity.

@dougwilson
Copy link
Contributor

However, on disk-backed and/or globally replicated stores the costs can be significant, such as connect-dynamodb

Regarding specific actions of stores, that is something disconnected from the general signaling of this module; for example perhaps a store may not need to do something on a touch or it shouls throttle like in your example. Is there a specific reason why this cannot be implemented in the store? Is the API between this module suiable enough to implement in the store side some like this which is a store-specific concern or does that API need to be enhanced to provide additional information in the touch request?

@huntharo

This comment was marked as resolved.

@dougwilson

This comment was marked as resolved.

@huntharo
Copy link
Author

Thank you. I just wrote the title to describe what I observed. The implementation of touch appears to only update the expiration time. I think we can change the title to indicate that this is a proposed option to allow reducing how frequently only the expiration is updated.

I'm not sure what to call it?

@dougwilson
Copy link
Contributor

dougwilson commented May 14, 2022

It seems the question (so issue title) at hand is something along the lines of "how to reduce writes to the session storage" maybe? That's at least what it seems. Is there a specific reason why this cannot be implemented in the store? Is the API between this module suiable enough to implement in the store side some like this which is a store-specific concern or does that API need to be enhanced to provide additional information in the touch request?

@huntharo
Copy link
Author

I believe the store protocol has enough info to skip the write. That was initially what I looked at but it occurred to me that this is much more generic and really applies to all stores.

Consider these bizarre cases that happen now:

  • Loading a webpage that makes 100 total resource requests over http/2 (all in parallel) on a 1000 instance express farm (I have this config)
    • Each express instance reads the session
    • Each express instance does a touch to update the expiration... this is a race condition - 100 touches will be done nearly simultaneously and all we can guarantee is that most recent timestamp is unlikely to be the last one written to the store - Which is "weird"?
    • Why try to write the expiration 100 times without guarantee of writing the most recent time?
  • Loading a webpage that makes 100 total resource requests over http/2 (all in parallel) on a 1 instance express farm
    • In this case this single instance does not keep track of the fact that it has already made 1, then 2, then 99 pending calls to the session store
    • All 100 requests to update the same value are pushed through to the store even though it's possible to deduplicate these so that only one pending touch call is made at a time per session or so that only one save call is made at a time per session (else there can even be a race where the 2nd call gets overwritten)

I do not suggest changing these behaviors by default because some may be relying on them. But I do suggest that there should be options to control them for those who would like to change how these odd cases work.

I mention these because it shows that the current behavior of touch is not well defined.

@dougwilson
Copy link
Contributor

Yes, I can see that it could perhaps apply to more stores than one -- but a module published as a decorator would then make that work without adding it here or breaking the contract between this module and the store (that contact is either save or touch is always called when there is activity). In your example, what are the resources? You should only configure this module to load the session if you need it -- you should not have this middleware before static resources unless they are served conditionally on the store data. As for race conditions, how that works and such is very store-technology-specific, which is why it is left to the store modules, with this module providing all the data that could be needed to make such a decision. Adding an option to withhold data from the store module is definitely not something we want to do -- the idea is that the store has all the data no matter what options the user has configured for this module's behavior.

@huntharo
Copy link
Author

Shouldn't touch and resave have never been added? Those are also decisions that could be made by the store yet they are provided here.

It seems odd to draw the line that adding features (selectable by consumers of this module) that control those touch and re-save are not a good fit but the feature itself is a good fit even though it contradicts the store having all the info.

@dougwilson
Copy link
Contributor

Hi @huntharo you are correct, and this is planned to be fixed in the next major version of this module. There is a big mess of options and behaviors in 1.x that makes it really hard to use. We are really trying to clean it up and continuing to pile on more and more behavior options is just the wrong directions we are trying to turn around the module for, which is why we are trying to really scrutinize these types of changes and really ask why it cannot be done in the store and to understand why that would be so we can make sure there is the right APIs here so the store can make all the decisions.

@ragesoft
Copy link

Hi @dougwilson , thx for your support and maintaining this module!
As of the latests release 9 days ago, i thought my problem with "resave" each and every request of the client is gone (problem as describted by this topic).

Fix resaving already-saved new session at end of request

I faced this problem on my dev environement (heroku) with the "mysql" session store. I can make some actions in my app, but i hit the hour request limit of free sql db releay fast. deep dive in query log shows my that on each single request 2 session queries are triggerd:
SELECT dataAS data,expiresas expires FROMsessionsWHEREsession_id = 'XXX'
UPDATE sessionsSETexpires= 1653655017 WHEREsession_id = 'XXX'

even if there is no other query / updated needed.

Maybe you are right and the store must handle this. But so it has to be done by each and every store owner. So i need some workaround here.
I'm using https://github.com/chill117/express-mysql-session

:)
Greetings

@dougwilson
Copy link
Contributor

Hi @ragesoft 👋 . In your example, the query you are showing is not part of the resave feature, but rather that your store module is updating the expires column on your data store every time the session is accessed. That may be what you want, maybe not. Ideally the store would need to add an option on it's end to allow you to control that behavior. This module just sends a signal to the store module that the session was accessed; what the store decides is the more appropriate thing to do with that signal is left up to the store module. I hope that helps.

@dougwilson dougwilson closed this as not planned Won't fix, can't repro, duplicate, stale May 20, 2022
@yamade
Copy link

yamade commented Jul 5, 2022

Hi @dougwilson, I have same issue now. Can we just skip touch when rolling is false?
It means shouldTouch(req) will returns false when rollingSessions is false.

@dougwilson
Copy link
Contributor

There is a PR here: chill117/express-mysql-session#133

@expressjs expressjs locked and limited conversation to collaborators Jul 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants