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
+from ConfigParser import SafeConfigParser
+from sitescripts.web import url_handler
+from sitescripts.extensions.utils import getDownloadLinks
+
+links = {}
+
+@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):
+  parser = SafeConfigParser()
+  getDownloadLinks(parser)
+  result = {}
+  for section in parser.sections():
+    result[section] = parser.get(section, "downloadURL")
+  queue.put(result)
+
+update_links()