Posix fixes

[hasufell]

@Mistuke can you review?

This addresses https://github.com/hasufell/file-io/issues/4

[Mistuke]

Hi, I'd given feedback months ago on irc :)

I had said this looks fine, the only question I has that I don't think we set cloexec on normal files today but only network handles. But could be wrong so asked you to double check that.

[hasufell]

Note: It might also be a good idea to use cloexec (O_CLOEXEC) to prevent child processes from inheriting the file descriptors.

@Rufflewind added that

[Mistuke]

So I've double checked, today on MIO https://github.com/ghc/ghc/blob/90c5abd4581b404f715e72ad55303e18d0c31d68/libraries/base/GHC/IO/FD.hs#L196 we don't set this flag by default and require people to make an explicit call to setCloseOnExec on the FD.

I think this change in behavior might be problematic for programs depending on the default behavior. So aside from this the rest are fine.

[Rufflewind]

I think this change in behavior might be problematic for programs depending on the default behavior.

That is true, but the setCloseOnExec approach has the risk of race conditions. Close-on-exec should have been the default from the start :(

A safe but ugly option would be to just make closeOnExec :: Bool an explicit argument, or rename the API to openFileWithCloseOnExec or something as a reminder.

[Mistuke]

I think this change in behavior might be problematic for programs depending on the default behavior.

That is true, but the setCloseOnExec approach has the risk of race conditions. Close-on-exec should have been the default from the start :(

I don't disagree with that it would have been a better default, but the context of this library is just to provide an abstraction layer to support OsPath while it's not merged into Base/GHC. In that regard I don't think we should be changing the behavior of programs. So what worked before should still work after using OsPath.

A safe but ugly option would be to just make closeOnExec :: Bool an explicit argument, or rename the API to openFileWithCloseOnExec or something as a reminder.

I wouldn't want an extra argument since that prevents this OsPath from being a drop in replacement. I'm not opposed to a new function with the explicit change. This isn't the first time this was brought up, just one such case is https://mail.haskell.org/pipermail/haskell-cafe/2015-July/120523.html but as far as I'm aware this was never brought to the CLC because there wasn't a clear consensus.

my worry here is that by changing the default it'll become harder to support both OsPath and String versions at the same time and people just trying it out can have their program that used to work misbehave.

I think this should be brought up as a GHC ticket and discussed there, and I definitely support the change there. (but like I said, I'm not opposed to the additional API for OsPath).

[hasufell]

I think @Rufflewind already released that change as part of directory package, which can't depend on this library, but has to inline it.

https://github.com/haskell/directory/commit/0ff4e13f1f51234fe047b4bc1ca60e567afd0c36

[Mistuke]

I think @Rufflewind already released that change as part of directory package, which can't depend on this library, but has to inline it.

haskell/directory@0ff4e13

That's quite unfortunate, I don't think this should have been decided at this level as it has wider effect than just directory.

But I'm not going to hold up this PR due to it, there will need to be a discussion when/if this ever gets merged into GHC.

For now could you provide a way to clear the flag from FDs so people can inherit the FD if required? base has no such method. So an inverse of https://github.com/ghc/ghc/blob/6b92b47fa2386ccb2f8264110ff7a827958fb7bf/libraries/base/System/Posix/Internals.hs#L356 That is, use FCNTL to clear the bits, then should be good to go.

[Rufflewind]

That's quite unfortunate, I don't think this should have been decided at this level as it has wider effect than just directory.

I am confused why the choices directory makes for an internal function matter to the broader ecosystem. directory only uses Internal.Posix.openFileForRead to implement copying.

[hasufell]

I think file-io maybe should be more conservative, since it's exposed API, so I will drop this part.