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

Fixes race condition issue when re-saving facebook access token #164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

heyman
Copy link

@heyman heyman commented Aug 13, 2012

Hi!

We're spawning a celery task when a user signs in, in order to re-fetch some data from facebook. In this task we use the access token from socialregistration.contrib.facebook. Sometimes we're getting an error because the user doesn't have an access token (even though they've had it on previous sign ins).

I believe this fixes the issue with non existing access tokens (since the delete & insert is replaced with an update if the access token already exists).

There's still a race condition where our task might get the old access token, but that is much less of an issue compared to not getting an access token at all. This could be solved by calling the django.contrib.auth.login() function after the socialregistration.signals.login is triggered. As far as I can tell this change should be OK with the contrib.facebook, but I'm not enough familiar with the code to know if this would break any other services, or any API promises.

@travisbot
Copy link

This pull request passes (merged e7434f5 into a4b67bb).

@flashingpumpkin
Copy link
Owner

Hey

Updating the token in place makes sense. I shall merge it in (and probably add the same change to the other OAuth consumers). Thanks! :)

@heyman
Copy link
Author

heyman commented Aug 29, 2012

Great! What do you reckon about calling django.contrib.auth.login() after sending the socialregistration.signals.login signal (https://github.com/flashingpumpkin/django-socialregistration/blob/master/socialregistration/views.py#L290) to solve the whole race condition issue?

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.

3 participants