Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyword shield doesn't work #2596

Open
mahrud opened this issue Sep 5, 2022 · 12 comments
Open

Keyword shield doesn't work #2596

mahrud opened this issue Sep 5, 2022 · 12 comments
Assignees

Comments

@mahrud
Copy link
Member

mahrud commented Sep 5, 2022

The documentation claims:

shield x -- executes the expression x, temporarily ignoring interrupts.

Unless I'm misunderstanding the page, which doesn't include any examples, shield doesn't seem to be working:

i1 : f = t -> shield sleep t;

i2 : f 10
^Cstdio:2:1:(3): error: interrupted

i3 : f = t -> ( alarm 1; shield sleep t );

i4 : f 5
stdio:4:1:(3): error: alarm occurred
@DanGrayson
Copy link
Member

Yup, it seems that that doesn't work. Good catch.

@mahrud
Copy link
Member Author

mahrud commented Sep 5, 2022

The interrupt mechanism in the interpreter is pretty complicated, so I doubt this will be easy to do for a volunteer. The relevant code is in evaluate.d and the most recent changes to it are from 13 years ago by you (there are no other contributors).

@pqcfox
Copy link

pqcfox commented Feb 13, 2023

If this is still available, I'd love to take a swing at it!

@DanGrayson
Copy link
Member

I've assigned you, thanks for volunteering!

@pqcfox
Copy link

pqcfox commented Feb 20, 2023

Of course, thanks so much!

@pqcfox
Copy link

pqcfox commented Jul 2, 2023

Finally got to figuring out the issue here, my apologies for the delay!

This is what I've been able to surmise is happening during e.g. shield sleep 5:

  • shieldfun sets the interruptShield flag and executes the sleep 5 expression
  • the sleep 5 expression in turn calls sleep(2) which suspends the thread
  • the SIGINT is received, waking the prompt thread and passing execution into interrupt_handler
  • interrupt_handler sees that interruptShield is set, and correspondingly sets interruptPending
  • control then passes immediately back to sleep(2), which passes control flow back to shieldfun
  • shieldfun turns off interruptShield and stores interruptPending into interruptFlag
  • evalraw sees interruptFlag is set and gives the exception

The "core" issue here seems to just be that the sleep concludes immediately when SIGINT is received. There's two possible fixes I thought of--if either of them seem useful, I can write them up real quick and share a PR:

  1. Make both the sleep and nanosleep keywords both use nanosleep(2) directly, saving the return value (a timespec with the remaining amount of sleep time before the interrupt) somewhere interrupt_handler can access it, and having interrupt_handler resume sleep if it sees that value set.
  2. Change sleep and nanosleep to manually mask signals before execution and unmask signals after.

Do either of these seem like a useful approach I should try implementing?

@pqcfox
Copy link

pqcfox commented Jun 19, 2024

(Wanted to quickly follow up on this, in case it's still relevant--would either of these solutions help?)

@d-torrance
Copy link
Member

Yes, I think it's definitely still relevant! I'm not sure which approach is better.

Are sleep and nanosleep the only things for which shield isn't working? It seems to be doing the correct thing for engine computations.

@pqcfox
Copy link

pqcfox commented Jun 21, 2024

I'll make a PR in that case!

I'm not sure if there's any other similar issues with shield (I haven't seen any), but @mahrud would know better than me.

@mahrud
Copy link
Member Author

mahrud commented Jun 22, 2024

Change sleep and nanosleep to manually mask signals before execution and unmask signals after.

Wouldn't it be simpler to mask signals when shieldfun starts?

Hopefully this will cover shielding calls to external libraries as well.

@pqcfox
Copy link

pqcfox commented Jun 24, 2024

That sounds like a better plan--will give that a shot :)

@mahrud
Copy link
Member Author

mahrud commented Oct 26, 2024

By the way, this didn't work as early as version 1.5, released in 2012!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants