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

fix: Use LRUCache for extendSessionOnUse #8683

Open
wants to merge 4 commits into
base: alpha
Choose a base branch
from

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Jul 6, 2023

Pull Request

Issue

Currently, extendSessionOnUse functions as a debounce, and does not clear the throttle store.

Closes: #8682

Approach

  • Change timeout logic (debounce) to a throttle, where the first session call is executed, and then subsequent calls do not extend
  • Sessions are no longer looked up and extended if they have been pulled from the session cache, as the only way they get into the session cache in the first place, is if they have been used recently (I believe it's safe to assume they have been extended already)

Tasks

  • Add new Parse Error codes to Parse JS SDK

@parse-github-assistant
Copy link

Thanks for opening this pull request!

@dblythy dblythy changed the title Correctly throttle extendSessionOnUse fix: Correctly throttle extendSessionOnUse Jul 6, 2023
@dblythy
Copy link
Member Author

dblythy commented Jul 6, 2023

@Moumouls could you add your thoughts?

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Patch coverage: 80.00% and project coverage change: -0.19 ⚠️

Comparison is base (c9b5971) 94.37% compared to head (9de7411) 94.19%.

❗ Current head 9de7411 differs from pull request most recent head ba02bb4. Consider uploading reports for the commit ba02bb4 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8683      +/-   ##
==========================================
- Coverage   94.37%   94.19%   -0.19%     
==========================================
  Files         185      185              
  Lines       14761    14758       -3     
==========================================
- Hits        13931    13901      -30     
- Misses        830      857      +27     
Impacted Files Coverage Δ
src/Auth.js 98.46% <80.00%> (-0.40%) ⬇️

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/Auth.js Outdated Show resolved Hide resolved
@dblythy dblythy requested a review from Moumouls July 6, 2023 13:12
@dblythy dblythy changed the title fix: Correctly throttle extendSessionOnUse perf: use LRUCache for extendSessionOnUse Jul 7, 2023
@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title perf: use LRUCache for extendSessionOnUse perf: Use LRUCache for extendSessionOnUse Jul 7, 2023
@mtrezza
Copy link
Member

mtrezza commented Jul 7, 2023

Is this only a perf PR or does it actually fix a bug, or both - for the changelog entry.

Copy link
Member

@Moumouls Moumouls left a comment

Choose a reason for hiding this comment

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

LGTM

@Moumouls
Copy link
Member

Moumouls commented Jul 7, 2023

Bug and perf i think @mtrezza

@mtrezza
Copy link
Member

mtrezza commented Jul 7, 2023

Then what would be a better PR title, to inform developers what the bug is.

@mtrezza
Copy link
Member

mtrezza commented Jul 14, 2023

@dblythy Could you rewrite the PR title? If the PR includes a bug fix and a perf improvement, then the PR is a fix since that is higher ranked and the perf improvement could be added as additional note.

@dblythy dblythy changed the title perf: Use LRUCache for extendSessionOnUse fix: Use LRUCache for extendSessionOnUse Jul 14, 2023
@mtrezza
Copy link
Member

mtrezza commented Mar 24, 2024

@Moumouls Do you think you could take a look at this Auth thing as well? We just need to resolve the conflict to merge this, but not sure how to do that. If you could make a suggestion I'd commit the change.

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.

extendSessionOnUse does not clear memory
3 participants