[PATCH] Futures: Avoid creating the worker pool more than once

[PATCH] Futures: Avoid creating the worker pool more than once

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 49 bytes --]

Any objections to applying this fix?

     Mark


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Futures: Avoid creating the worker pool more than once --]
[-- Type: text/x-diff, Size: 1661 bytes --]

From ff2c84b118b68020805eae93f71c3a8fd13ca3d9 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Tue, 6 Nov 2012 18:03:39 -0500
Subject: [PATCH] Futures: Avoid creating the worker pool more than once.

* module/ice-9/futures.scm (%create-workers!): Check to make sure the
  worker pool hasn't already been created within the critical section.
---
 module/ice-9/futures.scm |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
index 0f64b5c..afdf48f 100644
--- a/module/ice-9/futures.scm
+++ b/module/ice-9/futures.scm
@@ -158,13 +158,19 @@ touched."
 
 (define (%create-workers!)
   (lock-mutex %futures-mutex)
-  (set! %workers
-        (unfold (lambda (i) (>= i %worker-count))
-                (lambda (i)
-                  (call-with-new-thread process-futures))
-                1+
-                0))
-  (set! create-workers! (lambda () #t))
+  ;; setting 'create-workers!' to a no-op is an optimization, but it is
+  ;; still possible for '%create-workers!' to be called more than once
+  ;; from different threads.  Therefore, to avoid creating %workers more
+  ;; than once (and thus creating too many threads), we check to make
+  ;; sure %workers is empty within the critical section.
+  (when (null? %workers)
+    (set! create-workers! (lambda () #t))
+    (set! %workers
+          (unfold (lambda (i) (>= i %worker-count))
+                  (lambda (i)
+                    (call-with-new-thread process-futures))
+                  1+
+                  0)))
   (unlock-mutex %futures-mutex))
 
 (define create-workers!
-- 
1.7.10.4

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

Here's an improved version the patch that gracefully handles the case
where creation of the worker pool is unsuccessful due to an exception or
cancelled thread.

What do you think?

    Mark


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] Futures: Avoid creating the worker pool more than once --]
[-- Type: text/x-diff, Size: 2039 bytes --]

From b0d936a348b916e73e9071abeb7baae3d7c126d3 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Wed, 7 Nov 2012 08:39:42 -0500
Subject: [PATCH] Futures: Avoid creating the worker pool more than once.

* module/ice-9/futures.scm (%create-workers!): Use 'with-mutex' in case
  an exception is thrown.  Within the critical section, check to make
  sure the worker pool hasn't already been created by another thread.
---
 module/ice-9/futures.scm |   24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
index 0f64b5c..7fbccf6 100644
--- a/module/ice-9/futures.scm
+++ b/module/ice-9/futures.scm
@@ -19,6 +19,7 @@
 (define-module (ice-9 futures)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-9)
+  #:use-module (ice-9 threads)
   #:use-module (ice-9 q)
   #:export (future make-future future? touch))
 
@@ -157,15 +158,20 @@ touched."
 (define %workers '())
 
 (define (%create-workers!)
-  (lock-mutex %futures-mutex)
-  (set! %workers
-        (unfold (lambda (i) (>= i %worker-count))
-                (lambda (i)
-                  (call-with-new-thread process-futures))
-                1+
-                0))
-  (set! create-workers! (lambda () #t))
-  (unlock-mutex %futures-mutex))
+  (with-mutex
+   %futures-mutex
+   ;; Setting 'create-workers!' to a no-op is an optimization, but it is
+   ;; still possible for '%create-workers!' to be called more than once
+   ;; from different threads.  Therefore, to avoid creating %workers more
+   ;; than once (and thus creating too many threads), we check to make
+   ;; sure %workers is empty within the critical section.
+   (when (null? %workers)
+     (set! %workers
+           (unfold (lambda (i) (>= i %worker-count))
+                   (lambda (i) (call-with-new-thread process-futures))
+                   1+
+                   0))
+     (set! create-workers! (lambda () #t)))))
 
 (define create-workers!
   (lambda () (%create-workers!)))
-- 
1.7.10.4

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Daniel Hartwig]

On 7 November 2012 21:46, Mark H Weaver <mhw@netris.org> wrote:
> Here's an improved version the patch that gracefully handles the case
> where creation of the worker pool is unsuccessful due to an exception or
> cancelled thread.
>
> What do you think?

Looks clean.  Nice work picking up on this race condition.

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Ludovic Courtès]

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

> From b0d936a348b916e73e9071abeb7baae3d7c126d3 Mon Sep 17 00:00:00 2001
> From: Mark H Weaver <mhw@netris.org>
> Date: Wed, 7 Nov 2012 08:39:42 -0500
> Subject: [PATCH] Futures: Avoid creating the worker pool more than once.
>
> * module/ice-9/futures.scm (%create-workers!): Use 'with-mutex' in case
>   an exception is thrown.  Within the critical section, check to make
>   sure the worker pool hasn't already been created by another thread.
> ---
>  module/ice-9/futures.scm |   24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
>
> diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
> index 0f64b5c..7fbccf6 100644
> --- a/module/ice-9/futures.scm
> +++ b/module/ice-9/futures.scm
> @@ -19,6 +19,7 @@
>  (define-module (ice-9 futures)
>    #:use-module (srfi srfi-1)
>    #:use-module (srfi srfi-9)
> +  #:use-module (ice-9 threads)
>    #:use-module (ice-9 q)
>    #:export (future make-future future? touch))

I just realized that this patch introduces a circular dependency between
the two modules.

Can you think of a way to avoid it?

Thanks,
Ludo’.

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Mark H Weaver]

Hi Ludovic,

Sorry for not responding to this earlier.

ludo@gnu.org (Ludovic Courtès) writes:
> Mark H Weaver <mhw@netris.org> skribis:
>
>> diff --git a/module/ice-9/futures.scm b/module/ice-9/futures.scm
>> index 0f64b5c..7fbccf6 100644
>> --- a/module/ice-9/futures.scm
>> +++ b/module/ice-9/futures.scm
>> @@ -19,6 +19,7 @@
>>  (define-module (ice-9 futures)
>>    #:use-module (srfi srfi-1)
>>    #:use-module (srfi srfi-9)
>> +  #:use-module (ice-9 threads)
>>    #:use-module (ice-9 q)
>>    #:export (future make-future future? touch))
>
> I just realized that this patch introduces a circular dependency between
> the two modules.

Why is this a problem?  I think circular dependencies are fine as long
as there is not a cycle composed entirely of syntactic keyword
dependencies.

After introducing this circular dependency, I successfully bootstrapped
from scratch without difficulties.

What am I missing here?

      Mark

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Ludovic Courtès]

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

> Why is this a problem?  I think circular dependencies are fine as long
> as there is not a cycle composed entirely of syntactic keyword
> dependencies.

You’re right that it’s fine in some cases, but in general sometimes
works “by chance”, and should be avoided as a rule of thumb.

Ludo’.

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> +  ;; setting 'create-workers!' to a no-op is an optimization, but it is
> +  ;; still possible for '%create-workers!' to be called more than once
> +  ;; from different threads.  Therefore, to avoid creating %workers more
> +  ;; than once (and thus creating too many threads), we check to make
> +  ;; sure %workers is empty within the critical section.

Do you have a scenario where this happens?

Ludo’.

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Mark H Weaver]

ludo@gnu.org (Ludovic Courtès) writes:

> Mark H Weaver <mhw@netris.org> skribis:
>
>> +  ;; setting 'create-workers!' to a no-op is an optimization, but it is
>> +  ;; still possible for '%create-workers!' to be called more than once
>> +  ;; from different threads.  Therefore, to avoid creating %workers more
>> +  ;; than once (and thus creating too many threads), we check to make
>> +  ;; sure %workers is empty within the critical section.
>
> Do you have a scenario where this happens?

I'd hoped that the comment above was clear, but let me try again.

If two threads simultaneously call 'make-future' before the worker pool
has been created, then both threads can call 'create-workers!' (before
it has been rebound), and from there both can enter '%create-workers!'.

At that point, unless one of the threads is killed, it is guaranteed
that both will enter the critical section: first one, and then the other
(after the first one has unlocked the mutex).  Thus, two worker pools
will be created (though the first worker pool will become inaccessible).

      Mark

Re: [PATCH] Futures: Avoid creating the worker pool more than once

[Ludovic Courtès]

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> Mark H Weaver <mhw@netris.org> skribis:
>>
>>> +  ;; setting 'create-workers!' to a no-op is an optimization, but it is
>>> +  ;; still possible for '%create-workers!' to be called more than once
>>> +  ;; from different threads.  Therefore, to avoid creating %workers more
>>> +  ;; than once (and thus creating too many threads), we check to make
>>> +  ;; sure %workers is empty within the critical section.
>>
>> Do you have a scenario where this happens?
>
> I'd hoped that the comment above was clear, but let me try again.

Yes, what wasn’t clear to me is how practical this is, and whether
somebody encountered that situation in real life.

Anyway, please apply.

Thanks,
Ludo’.