Make sure barback's FilePool doesn't take up *all* the available FDs.

Make sure barback's FilePool doesn't take up *all* the available FDs.

BUG=13752
R=alanknight@google.com

Committed: https://code.google.com/p/dart/source/detail?r=29021

[Alan Knight, 2013-10-21 18:41:33]

lgtm

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
File pkg/barback/lib/src/file_pool.dart (right):

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:87: if (_timer != null) {
It seems a bit awkward to have the explicit cancel here, where normally we call
_heartbeat, which also does it.

[nweiz, 2013-10-22 18:37:02]

Committed patchset #1 manually as r29021 (presubmit successful).

[nweiz, 2013-10-22 18:37:28]

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
File pkg/barback/lib/src/file_pool.dart (right):

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:87: if (_timer != null) {
On 2013/10/21 18:41:33, Alan Knight wrote:
> It seems a bit awkward to have the explicit cancel here, where normally we
call
> _heartbeat, which also does it. 

We want to avoid restarting the timer here, since there aren't any pending
listens that could cause a deadlock.

[Bob Nystrom, 2013-10-26 00:13:34]

Couple of nits, but LGTM.

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
File pkg/barback/lib/src/file_pool.dart (right):

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:101: void _heartbeat() {
How about "_restartTimer"? It wasn't obvious to me that "heartbeat" meant "reset
the timer" and not just some other kind of keep-alive indication.

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:168: "use for too long.", new
Trace.current().vmTrace);
This makes it sound like the error is not related to pending reads. Maybe,
"Failed to open file: too many files were still open."

[nweiz, 2013-10-28 23:56:43]

I'm compiling all of these changes into a single revision CL, by the way.

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
File pkg/barback/lib/src/file_pool.dart (right):

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:101: void _heartbeat() {
On 2013/10/26 00:13:34, Bob Nystrom wrote:
> How about "_restartTimer"? It wasn't obvious to me that "heartbeat" meant
"reset
> the timer" and not just some other kind of keep-alive indication.

Done.

https://codereview.chromium.org/28733009/diff/1/pkg/barback/lib/src/file_pool...
pkg/barback/lib/src/file_pool.dart:168: "use for too long.", new
Trace.current().vmTrace);
On 2013/10/26 00:13:34, Bob Nystrom wrote:
> This makes it sound like the error is not related to pending reads. Maybe,
> "Failed to open file: too many files were still open."

This is less applicable now, since the error comes from the generic Pool class.