Skip to content
This repository was archived by the owner on Jul 18, 2025. It is now read-only.

Conversation

@Reepca
Copy link
Contributor

@Reepca Reepca commented Jan 30, 2022

This is the proposed fix in #37.

Currently, if an exception is thrown in the initial fiber, RUN-FIBERS never
returns, because the atomic box RET is never written to. This change makes it so
RET is written to in case of both normal and exceptional exits, and any caught
exception is re-thrown in the context of the caller.
@aconchillo
Copy link
Collaborator

Thanks! This one won't probably make it to 1.1.0. I believe it makes sense but I'd like to take a closer look.

@emixa-d
Copy link
Collaborator

emixa-d commented Jul 23, 2022

A test case is missing, to aid with verifying whether the change works, to avoid regressions later, and to assist with verifying future changes in this area of the code.

(dynamic-wind
(const #t)
(lambda ()
(catch #t
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use catch, as it destroys all backtrace information. Instead use with-exception-handler which doesn't destroy it (maybe with a fallback to 'catch' for old versions of Guile if it's not Guile 2.2). If you go for with-exception-handler, also go for raise-continuable instead of raise-exception/raise, otherwise you accumulate raise-exception in the backtrace, which is harmless but noisy.

;; Could be that this fiber was migrated away.
;; Make sure to wake up the main scheduler.
(spawn-fiber (lambda () #t) scheduler))
(dynamic-wind
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spacing seems wonky, though maybe that's just a different tabs / space convention (does someone know the conventions used in Fibers?).

(('ok vals)
(apply values vals))
(('err args)
(apply throw args)))))))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raise-continuable I mentioned would go here.

@emixa-d emixa-d added bug needs-changes This PR isn't quite right yet. labels Sep 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

bug needs-changes This PR isn't quite right yet.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants