Fix crashing in Safari (LB-23) #43
Conversation
|
This doesn't affect memory usage from what I can see, I just had to fix this first to even look at that in Safari. After actually checking, Firefox is the only browser that uses an insane amount of memory for me (and doesn't release it). |
|
this is interesting. It was originally like you had it: e53aa5c I lowered the number of current queries by using |
|
Hm, well, JavaScript's event loop already acts as a queue, so I'm not sure I understand what the processing queue would do on top of that. I noticed there are some bugs in the way it handles |
| page += 1; | ||
| setTimeout(function() { getLastFMPage(page, reportPageAndGetNext) }, 0 + Math.random()*100); |
There was a problem hiding this comment.
The setTimeout isn't needed because the XHR request is already asynchronous.
There was a problem hiding this comment.
It's also what was causing more than 10 activeSubmissions at a time, since the code was incrementing activeSubmissions inside the setTimeout.
There was a problem hiding this comment.
This was here because originally we didn't have a limit on the number of active submission. This was a random pause between paging between last.fm pages. I wasn't really worried about having 11 or 12 activeSubmissions going, just as long as it was about 10 instead of about 100
This commit fixes the `activeSubmissions` counter, so that it no longer allows
more than 10 concurrent pages to be processed at a time, adds error handling to
the last.fm page requests, and introduces a few invariants:
* `getNextPagesIfSlots` is the only function that increments `page` and
`activeSubmissions`.
* `pageDone` is the only function that decrements `activeSubmissions` and
increments `numCompleted`.
* Page requests/submissions will be retried until they either complete or
are skipped (due to e.g. a 40x status code), at which point `pageDone` is
called.
|
This looks good, merged. thanks! |
HDFS utilities for modules and scripts
The importer crashed every single time when I tested it in Safari 8.08, so I was able to reproduce this really easily.
With the changes here, it didn't crash at all after 8 or 9 trials.
This is clearly a bug in Safari, since there's nothing invalid about the code in question. Luckily, the code doesn't actually do anything, so we can just remove it.
Why the code does nothing:
enqueueReportpushes a single item ontotoReport, then callsdispatch(), which passes that single item toreportScrobblesand immediately clearstoReport.So, there is never more than one item in
toReport, and the code is equivalent to just callingreportScrobbleson the item directly.If there used to be a purpose to that and you'd rather fix it than remove it, let me know and I'll try to fix it instead. :)