393562 - Cap the number of downloads we kqueue

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[Stuart Morgan]

While investigating bug 387085 I realized we might be running out of file descriptors (the max is 256), and wondering if we or a plugin were leaking them somewhere. Then it occurred to me that we are doing something worse--opening an unbounded number of file descriptors that last potentially the lifetime of the app and are recreated on re-launch--by kqueueing all the downloads. All someone has to do is download enough files without moving them or clearing the download list and they will be permanently out (or at the brink of being out) of file descriptors. Given some of the bugs we have had before about slowness in the manager, I'd be shocked if there aren't users having this problem right now.

We need to pick a conservative cap (25?), and make sure we never have more than the last n downloads kqueue'd. The rest won't magically update, but that's the price we'll have to pay; we can put guards in on the file-manipulating actions to check that they are really still there and have an alert if not (we may already have them). If we think it's worthwhile, we could split off a new bug to set up a notification watch on the download folder and fix up most of the rest of the downloads that way.

Marking this for 1.5.2, since it's really bad when it happens, and it's likely to be happening to some people right now.

Comment 1

[Stuart Morgan]

Having thought about this more, I'm coming to the opinion that instead we should make the file watcher internally track directories instead of files (still capped for sanity, but users are way less likely to hit that), and do the work internally to figure out who to actually notify so the API is unchanged.

Comment 2

[Stuart Morgan]

Attachment: fix

This does two things:
- Changes FCW to watch directories, rather than files (vastly reducing the number of kqueues in normal usage), and capping the number of watched directories just in case. This means we just watch deletes/moves/etc. now, rather than any changes, but that's all we actually used in practice anyhow.
- Makes the PCVs more robust against not knowing they are deleted, so the experience isn't too bad for anyone who downloads to more than 25 folders at once. It's not fantastic, but it's easy and better than nothing.

Comment 4

[Nick Kreeger]

Comment on attachment 278173 [details] [diff] [review]
fix

Looks very good. Sorry I couldn't wrap my brain around this until today. 

Finding the missing file in the directory (file) array is probably the most efficient solution,  nice work!

Comment 5

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278173 [details] [diff] [review]
fix

sr=pink

Comment 6

[Stuart Morgan]

Landed on trunk, MOZILLA_1_8_BRANCH, and CAMINO_1_5_BRANCH.