Please don't make the simple patch too complex to discuss. Our current policy is to accept patch in a simple process. And we don't want newbies found it's too hard to be reviewed in a just one line patch.
Could you please elaborate on your point in shorter description? Best regards. On Wed, Aug 20, 2025, 20:24 Maxime Devos <maximede...@telenet.be> wrote: > On 8/19/2025 1:39 AM, Nala Ginrut wrote: > > To my understand, the current activity is to full fill the worker queue > first, then execute them one by one. > However, if the worker count is zero, the fulfillment will always return > an empty queue, so that the existing futures can't be executed. > > Don't see whatever 'the fulfillment' is supposed to be, that term doesn't > appear in the code. > > Also, where did you get 'worker count is zero => always empty queue' from? > This isn't supported by the code, futures are added to queues whether there > are workers or not, and futures don't disappear from queues out of nowhere. > > Going through the code, there are two queues: the %futures queue, and the > %futures-waiting queue. Maybe you meant the second but it was really > non-obvious. (or maybe you meant the first) Also, there is no queue of > workers (there is a list of workers, but it is never actually used except > to check whether workers were created). So really, it is unclear what queue > you are referring to. > > Now, going through the code, there does appear to be an issue, and it is > related to emptiness of %futures (detailed at the end), with as result that > sometimes aren't executed, so if you replace 'always => sometimes', > 'workers => futures" and move 'if the worker count is zero' to the end' (*) > you are correct, but by then it's quite a different statement, and it still > doesn't indicate the cause. > > (*) e.g. "the queue is sometimes empty, with as a result that sometimes > futures can't be executed when the worker count is zero" > > (On how 'worker count zero->always empty queue & no execution' doesn't > follow) Let's go through it: > > 1. Make a future, in a situation where %worker-count is 0. > 2. The initial state of the future is 'queued' ( > > https://cgit.git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/futures.scm#n72 > ) > 3. This then is added to the queue: > > https://cgit.git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/futures.scm#n113 > (so, no empty queue!) > 4. (nothing happens yet, because there are no workers) > 5. Touch the future. > 6. Because the state of the future is not 'done' and not 'started', we > are in the 'else' case: > > https://cgit.git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/futures.scm#n254 > 7. So, (work) is performed. > 8. The queue isn't empty (queues don't spontaneously lose their > elements), so now (process-one-future) is performed. > 9. In process-one-future, the queue still isn't empty. > 10. It removed some future from the queue. Let's assume it's the > future we are interested in (if not, things are done in a loop, so should > get there eventually) > 11. The future isn't done or started, so we are in the 'else' case ( > > https://cgit.git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/futures.scm#n175 > ) > 12. It then is marked as 'started', and '(process-future! future)' is > performed. > 13. (process-future! ...) calls the thunk of future, and if completed, > stores the result and returns #true (and also signals the condition). In > case of suspension, it marks the future as 'queued' again > 14. (a) one option is that it is put back in the %futures queue (-> go > to next iteration, in which maybe we'll hit the bug described later) > 15. (b) the other option is that it is put in %futures-waiting > (without being put in %futures) > 16. When the waited-on future completes, it moves relevant futures > from %futures-waiting to %futures > > Note that none of the above steps so far depend on whether there are > workers or not (except the '_maybe_ hit the bug' side remark). > > But, there appears to be an incorrect check. Look at the (eq? 'done ...) > line in (work): > > > https://cgit.git.savannah.gnu.org/cgit/guile.git/tree/module/ice-9/futures.scm#n235 > > (define (work) > ;; Do some work while waiting for FUTURE to complete. > (lock-mutex %futures-mutex) > (if (q-empty? %futures) > (begin > (unlock-mutex %futures-mutex) > (with-mutex (future-mutex future) > (unless (eq? 'done (future-state future)) > (wait-condition-variable (future-completion future) > (future-mutex future))))) > (begin > (process-one-future) > (unlock-mutex %futures-mutex)))) > > The potential states are 'done', 'started' and 'queued'. The last case is > done wrong (it's ok with workers, but not in general) > > - if 'done', obviously it shouldn't wait. Pretty much finished! > (handled correctly) > > > - if 'started', some thread is performing the worker. So, we should > wait until it is completed or suspended. First case: (futures-completion > future), second case: (futures-mutex future) (handled correctly) > - if 'queued' and there are workers, then we can wait for a worker to > pick it up (handled correctly) (or we could do it ourself, but doesn't > really matter) > - if 'queued' and there are no workers, then we have to do it ourself! > (handled _incorrectly_!!!) > > Summary: done->no wait, started->wait, queued->no wait, last case not > handled generally correct. > > So, (unless (eq? 'done (future-state future)) ...) needs to be changed to > (when (eq? 'started (future-state future)) ...). Perhaps give that a try > (instead of the %worker-count adjustment). (^^^) > > I think the issue of the current implementation is to mix the two > different situations, thread-unsupported and single-CPU. The simplest fix > is to make sure there is always a worker count. > > No. You can't ensure there are always workers, because workers require > thread support, and threads aren't always supported. (Unless you intend to > move fibers into guile or something like that.) > > In status quo, '(A) multi-thread support, but zero workers' and '(B) no > thread support, so zero workers' presumably behave the same (because > %worker-count is the same in both). So, they presumably have the same set > of futures-related bugs. And while (A) can be modified to ensure >=1 > workers, this can't be done for (B). So, if there is a bug, the bug remains > for (B) after the 'adjust %worker-count patch'. > > As mentioned previously: > > Also, %worker-count can still be 0 now, in case no thread support is > available. So, when no thread support is available, the same would > presumably happen. So if it is a bug in (ice-9 futures), it is not clear > that it is actually fixed. > > Also, the other change (^^^) is quite simple too, and (assuming I didn't > miss something) works in general, not only in the 'has thread support' > situation (unlike the %worker-count adjustment patch), and unlike your > patch has no change in semantics (well, except in the sense that that bugs > are unintended semantics). So (assuming no mistakes), (^^^) is a bit > simpler (no semantics change), (^^^) handles more cases and (^^^) addresses > the root of the issue. > > Best regards, > Maxime Devos >
