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

addQuitProc now works with closures, and c, js(node/browser) backend; fix some bugs in testament #14342

Merged
merged 6 commits into from
Jun 16, 2020

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented May 14, 2020

  • fixes all issues in Documentation nonexistent quitprocs module #14331 (comment)
    (in particular ordering issue)
  • it's moved quitprocs under std (as for all new modules)
  • avoid the need for a different overload
  • remove the C limitation of 30 atExit calls; (it should now be more efficient although untested)
  • make it work for -d:js (node and browser)
  • add test

testament fixes

  • testament/testament r tfoo now correctly adds target-specific default options (for now just -d:nodejs for js), just like it does when running without r (eg the js test suite runs with -d:nodejs but it didn't add -d:nodejs for testament r); this was needed so that this works: nim r testament/testament r tests/stdlib/tquitprocs.nim (+ others)
  • minor refactors to avoid some code duplication around areas I had to change, and fixes formatting (missing space) when extra options are passed: tests/stdlib/tquitprocs.nim JS--lib:lib => tests/stdlib/tquitprocs.nim JS --lib:lib

caveat

caveat is pre-existing from when addQuitClosure was added, and seems consistent with the preexisting doc comment: In case of an unhandled exception the exit handlers should not be called explicitly, although I think behavior should be changed (in future PR)

if a registered quitProc raises during teardown, subsequent procs won't be called; one option (in subsequent PR) would be to enhance this by wrapping each call into a try ... except and keeping track of at least one raised; and then call quit(0) or quit(1) depending on that

CI failures unrelated

TODO for future PR's

  • revisit behavior when a registered proc raises
  • add a check in kochdocs that new modules are added only under std/; there's already all logic in place for globbing source files + checking against whitelists
  • add vmops for addQuitProc; I've been needing this a few times, eg in some patch I've added some counters (timers + stats etc) around key compiler procs and the timers should be shown at exit

@timotheecour timotheecour force-pushed the pr_fix_addQuitClosure branch from 7225082 to 96c8087 Compare May 14, 2020 01:31
@timotheecour timotheecour marked this pull request as ready for review May 14, 2020 03:47
lib/system.nim Outdated Show resolved Hide resolved
lib/std/quitprocs.nim Outdated Show resolved Hide resolved
@Araq
Copy link
Member

Araq commented May 14, 2020

Please don't merge the two separate mechanisms into one.

@zah
Copy link
Member

zah commented May 19, 2020

It would be quite useful if the same treatment is applied to procs such as setControlCHook.

@timotheecour timotheecour force-pushed the pr_fix_addQuitClosure branch from 6d37566 to 7347e15 Compare June 14, 2020 08:28
@timotheecour timotheecour force-pushed the pr_fix_addQuitClosure branch from 7347e15 to 6a9fbeb Compare June 16, 2020 04:02
@timotheecour
Copy link
Member Author

timotheecour commented Jun 16, 2020

@zah

It would be quite useful if the same treatment is applied to procs such as setControlCHook.

this is left as future work

@Araq
PTAL, all comments addressed.
order is sane so long you use addExitProc (whether using a regular proc or closure);
I'm deprecating addQuitProc because it shouldn't be in system in the first place, and because you can't guarantee a sane ordering (last in first out) if you mix regular vs closure procs; however existing code won't break.

@Araq Araq merged commit dfe51d1 into nim-lang:devel Jun 16, 2020
@timotheecour timotheecour deleted the pr_fix_addQuitClosure branch June 16, 2020 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants