Closed mman closed 7 years ago
The description is pretty clear, thanks for the report. Do you by any chance have:
No cloudcode, just an iOS SDK from September, I'm trying to gather how many session rows are duplicated and looks like quite a lot... I'm trying to find some recent duplicates so that I can find the matching HTTP calls...
I don't have the detailed HTTP logs but I think this is what it looks like:
Mar 15 05:08:30 7aebaf975668[870]: 107.77.213.176 - - [15/Mar/2017:04:08:30 +0000] "POST /xxx/login HTTP/1.1" 200 415 "-" "XXX/348 CFNetwork/808.2.16 Darwin/16.3.0"
Mar 15 05:08:30 7aebaf975668[870]: 107.77.213.176 - - [15/Mar/2017:04:08:30 +0000] "POST /xxx/login HTTP/1.1" 200 415 "-" "XXX/348 CFNetwork/808.2.16 Darwin/16.3.0"
Just for the record I have now downgraded back to 2.3.2 which I know is not generating the message as I'm not yet sure whether the error 209 is not logging my users out as a side effect.
I think the questions that I have for the moment:
Looks like duplicate sessions are generated by quick successive POST /login
HTTP requests. If this is indeed true, parse server should update the existing session and not create another row. Correct? What would be the fix here assuming client side (iOS app) can not be updated reasonably quickly?
Will the error 209 generated as a result of an API call cause the client parse SDK to logout the user? Or can it be considered harmless?
What would be the best way to fix the database? cleanup all duplicate sessions by deleting the one with older _updated_at timestamp for example?
thanks for your help, Martin
should update the existing session and not create another row
The problem with that is that when the login occurs twice, no session is recorded in the database yet.
Will the error 209 generated as a result of an API call cause the client parse SDK to logout the user
It should be 'harmless' but no guarantee for sure.
cleanup all duplicate sessions by deleting the one with older _updated_at timestamp for example
that seems reasonable. But you can't guarantee for sure if you won't delete the session token being used. As this is a race condition, there's no way to know for sure which session token is being used. from the _Session table.
Do you have a lot of duplicates in the DB?
I'm using this query to find out duplicates by I'm not a mongo expert so I have not yet found out how to actually count how many users are affected. Do you know how to modify the group aggregate to return how many sessions are duplicated?
db.getCollection('_Session').aggregate( { $group: { _id: { "installationId": "$installationId", "_p_user": "$_p_user" }, count: { $sum: 1}, docs: { $push: "$_id" }}}, { $match: { count: { $gt: 1 }}})
To confirm. I have now been able to reproduce the problem and it is indeed a bug in my iOS app where certain conditions trigger double login API call.
nice, you should fix it too :) we can find a workaround in the same time.
Bug fixed and retested in the app beta, but it will take many months before it will spread to the users.
In the meantime due to the nature of the bug hitting the /login API endpoint twice from the same device quickly many devices will be duplicating my session rows over time.
I see two ways forward:
1/ Investigating what exactly is happening on the Parse iOS SDK side when it receives error 209 as a result of any API call, to assess the potential damage to the client. If harmless leave as is and ignore. If not harmless go to 2/
2/ Modify Auth.js line 59
to not throw on finding multiple sessions for the same installation id and same user and proceed with one session.
What do you think would be the best way forward?
You could also add a unique index to _Session on installationId and _p_user but will need to fix your existing bugged sessions first.
This is also something that could be added to parse-server in a similar way to there being unique indexes on _User. It would have the benefit that the database could not get in a broken state and then I think it would be safe to remove the line from Auth.js
.
If the checking in Auth.js
is not doing anything useful then could be removed too as it looks more of a hindrance right now when the _Session collection is in a broken state.
That would be only on Installation, for uniqueness, as a user can have multiple sessions
I mean something like the following so only the compound index is unique:
db.getCollection("_Session").createIndex({"installationId": 1, "_p_user": 1 }, {unique:true})
This should allow a user to have multiple sessions as long as the installationId is different for each one.
Or would that not work?
uhm right, that could work :)
Creating a unique index across installationId
and _p_user
will definitely ensure there could be no duplicate session for the same installation, but what would happen in my case where two consecutive POST /login
API calls arrive at around the same time? as in my buggy app?
I assume the first /login
that gets a chance to write new session will succeed, and the second /login
will fail to insert session and will thus fail.
Do I understand it correct?
the second login would fail yes, and that's not what we want or do we?
I think it should ideally fail but not sure how your app will handle that.
We had a similar issue with creating duplicate users with the same anonymous id, in which case we added an index on _User. It was better to return an error and deal with it than have to deal with duplicates in the database down the line. However, we use cloud code for all client calls so it was easy to update the error handling for this.
If the login requests are concurrent then it is going to be difficult to prevent this happening in code as when parse-server creates the sessions there will be no duplicates as far as each concurrent thread is concerned. We would have to perform a query after session creation and if there is a duplicate then delete one of them. We would have to make sure both concurrent threads played nice with this deletion, i.e. they don't both delete their own session! The thread that has its session deleted would have to return the session of the other thread.
This is why unique indexes are simpler but it moves responsibility to the client.
Yes I would agree with that, but in a sense, login twice is not 'bad' it's still valid as any of the login call is successful. Which can be more problematic client-side as the SDK may log the user out.
OR we should treat session creations as 'upsert' instead of inserts no?
I'm not 100% sure on the parse rules for multiple logins from the same installationId. If it can only happen in this edge case of concurrent login then perhaps we can live with it as it is valid to have multiple sessions per _User anyway.
https://github.com/ParsePlatform/parse-server/blob/master/src/Auth.js#L59 Looking again at the code, if looks like there will be an error if there is a duplicate session token but that is not the case here. The error must be returned somewhere else.
The upsert may help but I'm not sure what happens when called concurrently.
Looking at the situation from the client side API I think the issue resolves around what should login do when there is already a logged in user.
Either silently succeed, or fail with "error: already signed in, logOut first before doing another login".
I understand all the reasoning and technical details behind this, and I'm fine with either solution. I'd say let's choose the one that makes things easier and more transparent to implement and understand later from the maintenance perspective.
We can definitely let go through when multiple valid sessions are found. That would solve the issue.
@mman do you want to try open a PR for that issue?
@flovilmart I have taken two days to let the idea settle down a bit and to think about potential implementation given my limited knowledge of the parse server internals.
I'm now inclined to suggest that the server should allow multiple sessions per user and device as there is no easy way to allow only one session per user / device without some kind of locking and/or making the second login fail and thus impose a change on the existing apps using existing SDK. So preserving the current client SDK behavior when the underlying parse server changes is a reasonable requirement.
If we agree on that I'll have to find how the server behaved in 2.3.2 which is the last version that I know works for me without generating error messages and any side effects.
I will not have a chance to take a look at this for the next two weeks though. If you have the fix in your head I'll be happy to review and deploy the master once the fix is in and quickly verify whether everything works as expected. Otherwise I'll try to take a look later.
thanks for your help, Martin
I'll have a look, we should properly handle those cases.
are we taking in consideration what we said in: https://github.com/parse-community/parse-server/issues/2255 ??
I was able to reproduce the bug in earlier versions, not in v2.3.7, but it came back again (can't reproduce it like the old way, but it seems it's back!) It's a little bit annoying, I had to restart my server twice, and it's in production :(
@RafaRuiz we need to let multiple sessions for a single installationId, this may happen if multiple login calls are run right after one other.
Perhaps we can investigate this $isolated operator
Just to continue the investigation, I have rebuilt my servers to 2.3.8 and the error message is still there. Looking at the code carefully again the code 209 with the identical exception error message is also thrown from handleMe
method in the UsersRouter.js (https://github.com/parse-community/parse-server/blob/29fec01a42dd58b05d0a3d8220df4555bb44d89a/src/Routers/UsersRouter.js#L43)
Could you please help me figure out which API call could lead to this? Enabling VERBOSE=1 does produce a lot of output but I'd need something that that would help me understand where is this coming from...
I've been doing my debugging but I can't send a pull request because I'm not really an expert in this, is anyone looking at this? I have to restart around every day or every two days, and users are just uninstalling my app:
Have you indexed correctly the _Session table?
@flovilmart the _Session table is indexed with
but it's not the only one, this is the list:
anything wrong with it?
Hey guys any update on this? We just launched a game using 2.3.8 and I think we're getting bitten by this too. In our case when the 209 response is returned the game starts a new session, effectively losing all their progress.
@derekvanvliet can you isolate the request/responses that trigger those 209?
@flovilmart Yes, here's an example of what I'm seeing in my log:
`[36mverbose[39m: REQUEST for [GET] /1/users/me: {} method=GET, url=/1/users/me, host=example.com, x-real-ip=xx.xx.xx.xx, x-forwarded-for=xx.xx.xx.xx, xx.xx.xx.xx, accept=/, accept-encoding=gzip, deflate, accept-language=pt-pt, content-type=application/x-www-form-urlencoded, user-agent=UE4-RUA3,UE4Ver(4.15.2-0+++UE4+Release-4.15), x-parse-application-id=MyAppId, x-parse-rest-api-key=MyRestApiKey, x-forwarded-port=443, x-forwarded-proto=https,
[31merror[39m: Error handling request: ParseError { code: 209, message: 'invalid session token' } code=209, message=invalid session token [31merror[39m: invalid session token code=209, message=invalid session token `
The first thing that stuck out to me is that a session token isn't included in the request. But I've reviewed our client side code several times now and I don't see a way that it is not sending a session token.
I don't see a way that it is not sending a session token
So that's probably in the SDK or a previous call would have overriden the session token, we're making progress. That's good.
Impossible to know which user made that request (as we don't have the token); What SDK are you using?
@flovilmart this is using the REST API
So, you're sending a call to /users/me without a sessionToken, not sure how to help...
Just to update you guys, I'm watching this and all my investigation is pointing towards this error message being generated via GET /users/me
.
The strange thing is that I'm seeing this less with the latest 2.3.8 without me changing anything, and I can't identify an API call in the nginx logs that would not return HTTP 1.1 200
when this message drops in the parse logs. In fact I don't see any API calls towards /users/me
in the logs at all. And I can't find a place where we would use /users/me in our code.
So I have not forgotten this, it just seems to be very hard to debug :)
@mman what SDK's are you using?
@flovilmart It's 1.8.5 because of it being the last that supports iOS 7 IIRC
Thanks for tracking that down, I know where to focus the investigation now. By any chance, can you find a faulty iOS installation that triggers that issue, and relaunch it with request tracing? https://github.com/parse-community/Parse-SDK-iOS-OSX/wiki/Network-Debug-Tool
I will not be able to alter the client side easily, but I can play with the server so I can dump the request headers and response on the parse-server side. The only missing point is how to track down where is the error message coming from. Simply enabling VERBOSE=1 produces a lot of output - which is expected - but I can't find a clue as to what request actually generated the error. I'm looking for something similar to Java's Exception.printStackTrace()
that dumps the call stack at the moment error is printed or some even more verbose option that would track the request processing in more detail in the server...
do we have something more detailed than VERBOSE=1?
You can probably put the express logger, but I don't believe we need more detailed logs. Seems to be because the client is not sending it's session token, which would be because the session token is not sent from the client.
For anyone that could be interested.
Since there's no fix for this yet, I created a python script that checks a random endpoint that never would return 209 and, if it does, it restarts the server.
crontab -e
: (checks every minute)
*/1 * * * * python /parse-server/PronuntiaTests/Pronuntia209.py
Pronuntia209.py
:
import requests
import json
import subprocess
import datetime
urlForTest = "https://pronuntiapp.com:1338/parse/functions/{yourCloudFunctionHere}";
payload = { }
headers = { 'X-Parse-Application-Id' : '{yourParseAppIdHere}',
'X-Parse-REST-API-Key' : '{yourParseAPIKeyHere}',
'Content-Type' : 'application/json',
'X-Parse-Session-Token' : '{INCLUDE A SESSION TOKEN, grab it from the table, be sure that it belongs to an user, otherwise it wont work' }
res = requests.post(urlForTest, data=payload, headers=headers)
result = json.loads(res.text);
message = "[" + str(datetime.datetime.now()).split('.')[0] + "] "
try:
code = result["code"]
if code == 209:
message += "GOT 209! restarting server"
// change this for your script to restart the server. In my case, the bin is forever, arg1 = restart, arg2 = location of the index.js
subprocess.check_output(['forever', 'restart', '/parse-server/index.js'])
else:
message += "GOT " + str(code) + ". Not restarting the server"
except KeyError:
message += "GOT unknown error, this is the result: " + str(result)
// This will write a file for you that indicates a timestamp and what happened (whether if was restarted or not)
text_file = open("/tmp/ParseStatus.txt", "a")
text_file.write(message + "\n")
text_file.close()
There is no fix (yet) because we're unsure what the issue is or what yields it.
@RafaRuiz Exactly as @flovilmart says. The problem is real, I don't know where is it coming from and what bad is it doing - if anything. I have it in production for two weeks now and it does not seem to have any negative side effects. In any case, restart is not making it disappear or show less.
@mman did you had old non révocable session tokens that you upgraded with parse-server and not parse.com?
Also, we dropped support for iOS 8 6 months ago in my current company, do you have any plan to bump the SDK and your minimum iOS version?
@flovimart I don't know about non-revocable sessions, but there definitely were users using the app with some old SDK against parse.com that at some point in time upgraded to the new SDK and new parse server. Assuming (read I have no idea) the new SDK simply took the saved settings on the device it might explain your hypothesis.... but... it's still confusing to me that I don't see any 4xx or 5xx error codes in the log in addition to 209, like the javascript error is being ignored in fact.
To answer the minimum iOS requirement. Given the nature of our app I intend to support iOS 7 as long as possible so I will either stick with old client SDK version or will roll our own in house fork to keep compatibility...
I forgot to mention that I have another app that is iOS8 + on iOS SDK 1.14.2 and that has been released from day one running against our own parse server and that works great and without any errors.
@mman So those 209 errors are only generated form requests coming from the old SDK?
I know that it's not fixed, I was trying to help you guys in case you don't want to have the server throwing 209 unexpectedly.
In my case the 209 are coming from anywhere, using JS SDK, Android SDK and iOS SDK
What endpoint is returning it? Is it in the auth middleware? As @mman mentions, with a recent iOS SDK it doesn't seem to be an issuez
Issue Description
I have started getting the error
invalid session token code=209, message=invalid session token
after upgrading to 2.3.7 earlier today. Looking at now closed bug #2255 and at the source code the root cause seems to be that there are two session ids present for the same user and same installation id created and updated around the same time.Not all API calls generate this error and not all users are affected.
Steps to reproduce
Not sure, I'm continuously updating parse-server every few weeks to keep with the latest releases, the message started popping up after upgrading to 2.3.7.
Logs/Trace
This is one of the requests that generates the error message:
This is the query for all sessions for that user
If you look at the session for installation id
bcf5b055-3b1c-4dc2-a405-5e04a5a1c3ad
they are actually just few milliseconds apart which looks weird...Please let me know if you need more details.