Issue 402 - Use a redirector script for downloads, not a direct link

sitescripts/extensions/web/downloads.py

 # coding: utf-8
 
 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2014 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
 # GNU General Public License for more details.
 #
 # You should have received a copy of the GNU General Public License
 # along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
 
 import re
 import time
 import posixpath
 import urlparse
-import multiprocessing
+import threading
+import traceback
 from ConfigParser import SafeConfigParser
 from sitescripts.web import url_handler
 from sitescripts.extensions.utils import getDownloadLinks
 
 links = {}
+UPDATE_INTERVAL = 10 * 60   # 10 minutes
 
 @url_handler('/latest/')
 def handle_request(environ, start_response):
-  global links

[Sebastian Noack, 2014/09/15 10:18:39]

links doesn't have to be declared as global, since this
function doesn't set that variable.

-  update_links()

[Sebastian Noack, 2014/09/15 10:18:39]

Since the links links are updated asynchronously in a subprocess the first
request will always result in a 404. Not to mention other scenarios, where
obsolete data are used.

Is it really worth delegating the generation of the links to a subprocess? 

[Wladimir Palant, 2014/09/15 11:39:52]

Yes, it is definitely worth delegating - it can take a while, particularly the
AMO server isn't always responsive. Note that the alternative would be delaying
a random request in order to update the links which would result in a pretty
horrible download experience.

[Sebastian Noack, 2014/09/16 09:33:40]

I see. But maybe it would be a better approach to generate the links every 10
minutes regardless whether there is a request, that the data are always
up-to-date when the request occurs.

[Wladimir Palant, 2014/09/16 14:26:17]

Any way to implement this without complicating things too much? I also wouldn't
want to have this process around persistently...

[Sebastian Noack, 2014/09/16 14:30:05]

Having that process run for as long as the main process, independently updating
the links is exactly what I would suggest. This shouldn't add any complexity.

-
   request = urlparse.urlparse(environ.get('REQUEST_URI', ''))
   basename = posixpath.splitext(posixpath.basename(request.path))[0]
   if basename in links:
     start_response('302 Found', [('Location', links[basename].encode("utf-8"))])
   else:
     start_response('404 Not Found', [])
   return []
 
-UPDATE_INTERVAL = 10 * 60   # 10 minutes
-update_queue = multiprocessing.Queue()
-last_update = -float('Inf')
-
-def update_links():
-  global UPDATE_INTERVAL, update_queue, links, last_update

[Sebastian Noack, 2014/09/15 10:18:39]

UPDATE_INTERVAL and update_queue don't have to be declared as global, since this
function doesn't set those variables.

-  while not update_queue.empty():
-    links = update_queue.get()

[Sebastian Noack, 2014/09/15 10:18:39]

You should use atomic get, to avoid race conditions if the queue changes between
the .empty() and .get() call:

while True:
  try:
    links = update_queue.get(block=False)
  except Block:
    break

[Wladimir Palant, 2014/09/15 11:39:52]

There is no race condition here: elements can only be added to the queue, this
is the only code that will remove them. This means, if update_queue.empty()
returns False then the element won't be removed from the queue any more and
update_queue.get() will succeed. If it returns True however then something might
get added to the queue still - but we will simply get it next time this function
is called.

[Sebastian Noack, 2014/09/16 09:33:40]

In this particular case there might be no race conditions. But it still would be
cleaner and also slightly faster to use .get() in a non-blocking fashion rather
then checking for .empty() first.

-
-  now = time.time()
-  if now - last_update > UPDATE_INTERVAL:
-    last_update = now
-
-    process = multiprocessing.Process(target=_update_links, args=(update_queue,))
-    process.daemon = True
-    process.start()
-
-def _update_links(queue):
+def _get_links():
   parser = SafeConfigParser()
   getDownloadLinks(parser)
   result = {}
   for section in parser.sections():
     result[section] = parser.get(section, "downloadURL")
-  queue.put(result)
+  return result
 
-update_links()
+def _update_links():
+  global links
+
+  while True:
+    try:
+      links = _get_links()
+    except:
+      traceback.print_exc()
+    time.sleep(UPDATE_INTERVAL)
+
+t = threading.Thread(target = _update_links)
+t.daemon = True
+t.start()