-
Notifications
You must be signed in to change notification settings - Fork 82
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
Create output file atomically #221
base: master
Are you sure you want to change the base?
Conversation
@sol, thanks for the PR!
I don't see a problem with dropping GHC versions < 7 because we anyway do not test them. CI goes back to 7.0 only.
This could be good for code uniformity. At least if there is similar code in other places, it should be changed in sync.
I don't think that would be necessary. One question: Can the memory footprint of |
No, it doesn't. This patch creates a temporary file and later renames it. For that reason, the memory characteristics are unchanged. The downside of this approach is that we create that temporary file on the file system and trigger corresponding inotify events, which I don't really like. The pros are that this is a common and well understood approach to guarantee atomicity when creating files + it's easy to implement without the risk of unintentional side effects. An other approach would be to construct a A third and somewhat half measured approach would be to try do a little bit more work upfront (before opening the output file), so that the output file stays empty for a shorter period of time. This may or may not be enough to get away with it. I haven't looked into this at all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, if you rebase on master
now, you get CI up to GHC 9.6.1 alpha2.
src/Main.hs
Outdated
@@ -169,10 +173,12 @@ alex cli file basename script = do | |||
|
|||
scheme <- getScheme directives | |||
|
|||
let o_file_tmp = o_file ++ ".tmp" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this maybe more robust by using a proper temp file?
See e.g. https://hackage.haskell.org/package/base-4.3.0.0/docs/System-IO.html#v:openTempFile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made two changes:
- Use
openTempFile
- Append a
~
to the name of the temporary file
The rationale for (2) is so that the file is treated the same way as backup files by file watching utilities and the like.
c134931
to
66211b0
Compare
@andreasabel before moving forward with this, I will dogfeed it for a little bit longer. |
This change is useful if you both (a) run
alex
on modifications to.x
-files and (b):reload
GHCi on modifications to.hs
-files. It prevents that your GHCi session will see partial source files.As things are you will initially get an error of the form:
(as initially the output file is empty)
Considerations:
GHC < 6.10.1
(as we don't have compat code forbracket
and I'm not eager to burn cycles on that)..info
-files (personally however,.hs
-files is what I care about).--atomic-output
or something).Thoughts?