search

omab / django-social-auth

Django social authentication made simple
https://groups.google.com/group/django-social-auth
BSD 3-Clause "New" or "Revised" License
2.65k stars 757 forks source link

No timeouts when communicating with external APIs? #420

Closed johtso closed 12 years ago

johtso commented 12 years ago

During yesterday's Twitter outage the twitter API would occasionally just fail to respond. This meant that django-social-auth just left the connection open until the pipe broke.

Is django-social-auth able to timeout if an external API fails to respond?

omab commented 12 years ago

At which point did it hung? API call, redirect to Twitter login/allow app?

omab commented 12 years ago

@johtso, I've created a branch to track this issue, in that I've added a setting to define a timeout (SOCIAL_AUTH_URLOPEN_TIMEOUT) for urllib2.urlopen calls. Seems to work OK so far, so I'll merge it soon to master, could you give it a look?

mlavin commented 12 years ago

It's worth noting that the urlopen timeout is the socket connection timeout and does not handle read timeouts. This argument was also added in Python 2.6 but I'm not sure this project worked on Python 2.5 before this proposed change.

omab commented 12 years ago

Yeah, that's why it's not altered unless the setting is defined, maybe some extra warning in the doc is needed. Python version is not specified, but I don't mind excluding 2.5 out.

omab commented 12 years ago

Doc improve https://github.com/omab/django-social-auth/commit/627800702c3aa4e8c9dfc3b15257e383e3a8391c

mlavin commented 12 years ago

I don't see a problem with having it on by default. It's more that if server connection is established within the timeout then reading the content can still take beyond the timeout and lead to the same problem as originally reported.

uruz commented 12 years ago

I think, maybe we should switch to the perfect python-requests library, doing all the connections to external API through it and have a config for that library (and timeout within that config). It would be a lot of work though

mlavin commented 12 years ago

I love requests as much as the next person but it doesn't really solve the problem I'm describing either as you'll see in the timeout documentation note. Whether DSA needs to handle this case or simply live with this possibility is up to @omab. DSA can't make the Facebook or Twitter API work if their servers are down or overloaded. The current change is already a nice improvement and I didn't mean to imply otherwise.

omab commented 12 years ago

@uruz, I've thought about that but so far urlopen does the work quite well, so didn't wanted to do a that big change yet, and I don't think it's gonna happen in the near future. Also I've been thinking to use this recent lib https://github.com/litl/rauth/, which already uses requests and brings an easier support to call OAuth1/2 APIs.

Right now I'm happy with the current fix and will merge it to master today.

omab commented 12 years ago

Branch merged and released v0.7.2