Closed Bug 393562 Opened 14 years ago Closed 14 years ago

Cap the number of downloads we kqueue

Categories

(Camino Graveyard :: Downloading, defect)

x86
macOS
defect
Not set
critical

Tracking

(Not tracked)

RESOLVED FIXED
Camino1.6

People

(Reporter: stuart.morgan+bugzilla, Assigned: stuart.morgan+bugzilla)

References

Details

(Keywords: fixed1.8.1.8, Whiteboard: [camino-1.5.2])

Attachments

(1 file)

fix
15.40 KB, patch
nick.kreeger
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
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.
Flags: camino1.5.2+
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.
Attached patch fixSplinter Review
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.
Attachment #278173 - Flags: review?(nick.kreeger)
Severity: normal → critical
Duplicate of this bug: 387085
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!
Attachment #278173 - Flags: review?(nick.kreeger) → review+
Attachment #278173 - Flags: superreview?(mikepinkerton)
Comment on attachment 278173 [details] [diff] [review]
fix

sr=pink
Attachment #278173 - Flags: superreview?(mikepinkerton) → superreview+
Landed on trunk, MOZILLA_1_8_BRANCH, and CAMINO_1_5_BRANCH.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: fixed1.8.1.8
Resolution: --- → FIXED
Target Milestone: --- → Camino1.6
You need to log in before you can comment on or make changes to this bug.