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

Fix SaveOnExitFile in restored workspaces #5043

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

fingolfin
Copy link
Member

... and possibly more errors: we accidentally removed all exit handlers when quitting GAP, just before the workspace was saved, thus when restoring GAP from this session, those exist handlers were missing, including the one dealing with SaveOnExitFile.

This code looses the ability to install new exit handlers as part of an exit handler, but that seems like a rather bad (and pointless) idea to me anyway.

Fixes #5035

... and possibly more errors: we accidentally removed all exit handlers
when quitting GAP, just before the workspace was saved, thus when
restoring GAP from this session, those exist handlers were missing,
including the one dealing with `SaveOnExitFile`.

This code looses the ability to install new exit handlers as part of
an exit handler, but that seems like a rather bad (and pointless)
idea to me anyway.
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes backport-to-4.12 labels Sep 9, 2022
@ChrisJefferson
Copy link
Contributor

My thought was that AtExit functions should only be run once, so I didn't need to look after the list, but I see that's not true.

We might want to add something to do the docs of AtExit saying they may be run more than once (although now I think about this, this was always true with saved workspaces, so this isn't a new problem). I just went and checked and I think our current ones are all OK. The main one that worried me was TemporaryDirectory, but turns out there is already explicit code for this -- in system.g, CallAndInstallPostRestore clears GAPInfo.DirectoriesTemporary when we load a saved workspace.

@fingolfin fingolfin merged commit a469525 into gap-system:master Sep 9, 2022
@fingolfin fingolfin deleted the mh/fix-SaveOnExitFile branch September 9, 2022 18:10
@fingolfin
Copy link
Member Author

Backported to stable-4.12 via c1009a2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-4.12-DONE kind: bug Issues describing general bugs, and PRs fixing them release notes: use title For PRs: the title of this PR is suitable for direct use in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SaveOnExitFile does not work for quit;
2 participants