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

Avoid double info request for compound annotations #4619

Merged
merged 8 commits into from
May 19, 2020
Merged

Conversation

fm3
Copy link
Member

@fm3 fm3 commented May 14, 2020

Compound Annotations are created lazily, and then cached.
The frontend checks the permissions for logged-out users for an annotation in parallel with loading it. That’s why two info requests are sent.
If the second info request fires before the first one has completed, the compound annotation is created twice, and only one of the two will be in the (key-value) cache. That is the race condition.
They will have different skeleton tracing IDs.
If the frontend then assumes the wrong skeleton ID, the reverse lookup for permission check fails.

This PR changes the frontend to only check for logged-out users if the user is logged out, avoiding this double info request.

URL of deployed dev instance (used for testing):

Steps to test:

  • view compound tasks (of at least two instances), should not fail, network tab should show only one info request
  • viewing public annotations (on public datasets) while logged out should still work

Issues:


@fm3 fm3 requested review from philippotto and youri-k May 14, 2020 17:54
@MichaelBuessemeyer
Copy link
Contributor

@fm3 As we discussed I tried to make the error handling while loading the tracing better by displaying unhandled error messages better to the user:
Screenshot_20200514_212750

I'll push the changes for this to this branch

 * better displaying the error to the user
@philippotto
Copy link
Member

Compound Annotations are created lazily, and then cached.
If the second info request fires before the first one has completed, the compound annotation is created twice, and only one of the two will be in the (key-value) cache. That is the race condition.
They will have different skeleton tracing IDs.
If the frontend then assumes the wrong skeleton ID, the reverse lookup for permission check fails.

I don't really understand where these two info requests come from. I guess, one of them is getAnnotationInformation in the <SecuredRoute /> part which is also changed in this PR? Which info request is executed in parallel to that?

I also don't understand why returning false in serverAuthenticationCallback helps in case of compound annotations. 🤔 Wouldn't this show an unauthorized error if the race condition goes bad?

Sorry, I'm a bit lost here 🙈 I understand the bug from the description in your PR, but I don't understand how this matches to the code diff.

@fm3
Copy link
Member Author

fm3 commented May 15, 2020

Ok I guess the real question is: why is this auth callback code executed even if the user is logged in. My guess is that it should handle logged-out users.
It executes this info request, and in parallel the model initialization already does its own.
The diff in this PR prevents this auth check for the compound annotations (for they are always non-public and the get shouldn’t be necessary to determine that) but I suppose the real issue is elsewhere. I’ll have another look on monday, trying to figure out what this callback mechanism is supposed to do and what it actually does.

@fm3 fm3 requested a review from daniel-wer May 18, 2020 08:19
@fm3
Copy link
Member Author

fm3 commented May 18, 2020

Apparently, the serverAuthenticationCallback is always executed for the routes that have it. The reply is then ORed with the normal isAuthenticated. So this PR should work correctly. @daniel-wer @MichaelBuessemeyer do you think we should change the design in SecuredRoute completely so that the serverAuthenticationCallback is only called if isAuthenticated is false? Or do you think this solution is ok?

@fm3 fm3 removed the request for review from philippotto May 18, 2020 08:20
frontend/javascripts/messages.js Outdated Show resolved Hide resolved
@daniel-wer
Copy link
Member

@fm3 Thanks for looking and digging into this! I would say the more general and cleaner solution would be to only call the serverAuthenticationCallback if isAuthenticated is false. This should lead to fewer unnecessary requests, avoids the race condition in question and doesn't rely on assumptions of routes that are always non-public. It should be enough to change line 32 in secured_route.js to if (!this.props.isAuthenticated && this.props.serverAuthenticationCallback != null) { (untested) and revert the changes in the router.

Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

LGTM, works like a charm and I can only see one info request 👍

@fm3 fm3 merged commit be3fa73 into master May 19, 2020
@fm3 fm3 deleted the fix-annotation-cache branch May 19, 2020 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

View button for task instance does not work properly
4 participants