mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
123 stars 53 forks source link

Make NonAtomicRequestsForSafeHttpMethodsMiddleware thread-safe #1819

Open diox opened 4 years ago

diox commented 4 years ago

Discussion originated from https://github.com/mozilla/addons-server/pull/9548/files#r372398934

Our way of marking views as not needing atomic requests is not thread-safe. It modifies the function, which would be shared across threads. We never noticed, the timing to trigger the issue is a bit specific, but it's possible in theory so we should definitely fix it.

Switching that middleware to new-style middleware would allow us to write the code like this:

if is_method_safe and not transaction.get_connection().in_atomic_block:
    response = non_atomic_requests(get_response(request))
else:
    response = get_response(request)

Or something along those lines. If that doesn't work we'll have to give up on ATOMIC_REQUESTS setting and do the call to atomic() ourselves.

┆Issue is synchronized with this Jira Task

diox commented 4 years ago

Tested this and it doesn't work - the get_response() function is not the actual view the handler will execute, and therefore the special attribute it looks for doesn't exist.

diox commented 4 years ago

The way forward that I see right now is to re-implement ATOMIC_REQUESTS in a middleware. This is annoying though, because we still want non_atomic_requests() functionality, i.e. marking a view as not needing atomic requests regardless of the HTTP verb used.

That means we need to access the view function itself before deciding to wrap the call in an atomic() block. Unfortunately, in Django new-style middleware, __call__ never receives the original view func, it only receives the get_response wrapper. There is still a process_view callback available, and that one does have the original view function, but it's called after we've called all the middlewares, which would be too late - we'd already be in an atomic block or not at that point.

I'll keep digging... Alternatively, as discussed on IRC, we could stop using all this and explicitly wrap calls that need transactions in atomic blocks everywhere. That means going over a couple hundreds of functions and classes, though...

KevinMind commented 2 months ago

Old Jira Ticket: https://mozilla-hub.atlassian.net/browse/ADDSRV-27