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

This should run on the download server I think.

[Sebastian Noack, 2014-09-15 10:18:38]

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:31: global links
links doesn't have to be declared as global, since this
function doesn't set that variable.

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:32: update_links()
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?

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:47: global UPDATE_INTERVAL,
update_queue, links, last_update
UPDATE_INTERVAL and update_queue don't have to be declared as global, since this
function doesn't set those variables.

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:49: links = update_queue.get()
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]

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:32: update_links()
On 2014/09/15 10:18:39, Sebastian Noack wrote:
> 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? 

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.

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:49: links = update_queue.get()
On 2014/09/15 10:18:39, Sebastian Noack wrote:
> You should use atomic get, to avoid race conditions if the queue changes
between
> the .empty() and .get() call:

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:39]

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:32: update_links()
On 2014/09/15 11:39:52, Wladimir Palant wrote:
> On 2014/09/15 10:18:39, Sebastian Noack wrote:
> > 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? 
> 
> 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.

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.

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:49: links = update_queue.get()
On 2014/09/15 11:39:52, Wladimir Palant wrote:
> On 2014/09/15 10:18:39, Sebastian Noack wrote:
> > You should use atomic get, to avoid race conditions if the queue changes
> between
> > the .empty() and .get() call:
> 
> 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.

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.

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

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:32: update_links()
On 2014/09/16 09:33:40, Sebastian Noack wrote:
> 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.

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:04]

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5178081291534336/site...
sitescripts/extensions/web/downloads.py:32: update_links()
On 2014/09/16 14:26:17, Wladimir Palant wrote:
> On 2014/09/16 09:33:40, Sebastian Noack wrote:
> > 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.
> 
> Any way to implement this without complicating things too much? I also
wouldn't
> want to have this process around persistently...

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.

[Wladimir Palant, 2014-09-16 15:04:47]

On 2014/09/16 14:30:04, Sebastian Noack wrote:
> 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.

It will, as that process might die and will have to be respawned. And it will
use memory persistently.

[Wladimir Palant, 2014-09-16 19:15:21]

I learned a lot more about multithreading in Python than I wanted but I think
that the solution is cleaner now. The updates are now being triggered by a timer
and completely independent of the web handler. Also, the result is processed by
a callback - no more queue polling.

[Sebastian Noack, 2014-09-17 13:32:10]

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
sitescripts/extensions/web/downloads.py:41: def update_links():
Since you are apparently fine with keeping a thread around, why not just using a
thread instead a process (or a combination of both)?

This would be way simpler, and there isn't a real benefit of using a subprocess
(instead a thread) for the actual work here.

def _set_links():
  global links

  while True:
    links = _update_links()
    time.sleep(UPDATE_INTERVAL)
    
thread = threading.Thread(target=_set_links)
thread.start()

[Wladimir Palant, 2014-09-17 17:45:43]

Argh, should have actually sent that comment...

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
sitescripts/extensions/web/downloads.py:41: def update_links():
On 2014/09/17 13:32:11, Sebastian Noack wrote:
> Since you are apparently fine with keeping a thread around, why not just using
a
> thread instead a process (or a combination of both)?

That was the idea originally. However,
https://stackoverflow.com/a/2846695/785541 and other sources suggest that using
threads only makes sense for I/O operations in Python, any Python code running
still blocks the main thread.

[Sebastian Noack, 2014-09-17 17:53:42]

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
sitescripts/extensions/web/downloads.py:41: def update_links():
On 2014/09/17 17:45:44, Wladimir Palant wrote:
> On 2014/09/17 13:32:11, Sebastian Noack wrote:
> > Since you are apparently fine with keeping a thread around, why not just
using
> a
> > thread instead a process (or a combination of both)?
> 
> That was the idea originally. However,
> https://stackoverflow.com/a/2846695/785541 and other sources suggest that
using
> threads only makes sense for I/O operations in Python, any Python code running
> still blocks the main thread.

That is correct, but IMO not an issue here:

1. The reason why generating those links takes so long is only IO time.
2. As far as I know the server this is supposed to run on has only one CPU core
anyway.

As long as at least one of those assumptions hold true, there is no advantage of
subprocesses over threads. And even then it would be questionable if that
advantage justifies the added complexity.

[Sebastian Noack, 2014-09-17 18:13:53]

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
File sitescripts/extensions/web/downloads.py (right):

http://codereview.adblockplus.org/5747446760079360/diff/5673385510043648/site...
sitescripts/extensions/web/downloads.py:41: def update_links():
On 2014/09/17 17:45:44, Wladimir Palant wrote:
> any Python code running still blocks the main thread.

Note that CPU time is distributed across all threads, similar as when running
multiple processes with a single CPU. The main thread won't block until other
threads are finished.

[Wladimir Palant, 2014-09-17 19:04:24]

Ok, new version with more threading :)

[Sebastian Noack, 2014-09-17 19:06:44]

LGTM