Skip to content

Fix intense CPU usage when sessionToken is invalid in liveQuery #5126

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 18, 2018
Merged

Fix intense CPU usage when sessionToken is invalid in liveQuery #5126

merged 3 commits into from
Oct 18, 2018

Conversation

flovilmart
Copy link
Contributor

@flovilmart flovilmart commented Oct 18, 2018 •
edited
Loading

When sessionToken fetch failed through getAuthForSessionToken auth and userId would be undefined.

This PR ensure we do not throw an exception in this case.

@flovilmart flovilmart changed the title Ensure we bail out early when auth or userId are not provided (sessio… Ensure we bail out early when auth or userId are not provided Oct 18, 2018
@dplewis
Copy link
Member

dplewis commented Oct 18, 2018

I don’t think it’s needed but test case?

@flovilmart
Copy link
Contributor Author

@dplewis this is a case that was overlooked initially.

dplewis
dplewis previously approved these changes Oct 18, 2018
View reviewed changes
acinader
acinader previously approved these changes Oct 18, 2018
View reviewed changes
@flovilmart flovilmart dismissed stale reviews from acinader and dplewis via dc97391 October 18, 2018 01:52
@codecov
Copy link

codecov bot commented Oct 18, 2018 •
edited
Loading

Codecov Report

Merging #5126 into master will decrease coverage by <.01%.
The diff coverage is 95.45%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5126      +/-   ##
=========================================
- Coverage    93.8%   93.8%   -0.01%     
=========================================
  Files         123     123              
  Lines        8915    8923       +8     
=========================================
+ Hits         8363    8370       +7     
- Misses        552     553       +1
Impacted Files Coverage Δ
src/LiveQuery/ParseLiveQueryServer.js 87.46% <95.45%> (-0.33%) ⬇️
src/Adapters/Auth/httpsRequest.js 100% <0%> (+4.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c81290...dc97391. Read the comment docs.

flovilmart
flovilmart commented Oct 18, 2018
View reviewed changes
src/LiveQuery/ParseLiveQueryServer.js
// Check client sessionToken matches ACL
const clientSessionToken = client.sessionToken;
if (clientSessionToken) {
const { userId } = await this.getAuthForSessionToken(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was initally overlooked, the new implementation takes properly care of both tokens

@flovilmart flovilmart changed the title Ensure we bail out early when auth or userId are not provided Fix intense CPU usage when sessionToken is invalid in liveQuery Oct 18, 2018
@flovilmart
Copy link
Contributor Author

flovilmart commented Oct 18, 2018 •
edited
Loading

@westhom this is the fix you are looking for. Probably the behaviour will require tweaking. Invalid session tokens should not issue server calls for 10 minutes (they may call after) and you should see performance improvements at large.

@flovilmart
Copy link
Contributor Author

flovilmart commented Oct 18, 2018 •
edited
Loading

@dplewis @acinader what do you think of this implementation? and i added tests :)

acinader
acinader reviewed Oct 18, 2018
View reviewed changes
src/LiveQuery/ParseLiveQueryServer.js
);
const { auth, userId } = await this.getAuthForSessionToken(token);

// Getting the session token failed
Copy link
Contributor

Choose a reason for hiding this comment

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

fail fast

acinader
acinader approved these changes Oct 18, 2018
View reviewed changes
Copy link
Contributor

@acinader acinader left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm. a solution to an immediate problem that can help @westhom and live query.

@flovilmart flovilmart closed this Oct 18, 2018
@flovilmart flovilmart reopened this Oct 18, 2018
@flovilmart flovilmart merged commit 4b7037a into master Oct 18, 2018
@flovilmart flovilmart deleted the fix/exception-livequery branch October 18, 2018 11:21
UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
@flovilmart
Fix intense CPU usage when sessionToken is invalid in liveQuery (pars…
6be356d 
…e-community#5126)

* Ensure we bail out early when auth or userId are not provided (sessionToken fetch is invalid)

* Adds changelog

* better handling of session token errors and client tokens
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acinader acinader acinader approved these changes

@dplewis dplewis dplewis left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flovilmart @dplewis @acinader