prevent crash for WinIO in GHC 9+

[ruifengx]

As reported in #25, hSilence etc. currently fail with --io-manager=native on Windows. According to GHC issue #22146, the hDuplicateTo API is not supported for the new IO manager (which I believe will become the default for future GHC), and therefore hSilence etc. need some patch to avoid crashing with the new IO manager on Windows.

I do not know a workaround for hijacking an existing normal file handle, but for the common use case (to silence the standard handles stdin/stdout/stderr), I figured out a workaround. I use SetStdHandle to overwrite and recover the handle, and use CreateFile to get back the console handle. Please refer to the linked Microsoft documentation for details.

This PR does not change the behaviour for GHC 8 and prior, nor GHC 9+ on native POSIX systems (Linux, macOS, etc.) or on Windows but with --io-manager=legacy (the default). In other words, the only combination that gets affected is GHC 9+ on Windows with --io-manager=native, for which the previous behaviour was to crash the whole program. The new behaviour correctly takes care of the three standard handles, but does nothing for normal file handles (without crashing).

I have tested the implementation on my local PC (GHC 9.0.2, Windows 11), and confirmed it works as intended. However, perhaps we should also add a test in the repo so that it could be verified in CI.

[andreasabel]

I have tested the implementation on my local PC (GHC 9.0.2, Windows 11), and confirmed it works as intended.

What is the magic incantation to test this? I tried these

$ cabal test --test-options='+RTS' --test-options='--io-manager=native'
$ cabal run spec-generic -- +RTS --io-manager=native

but I always get

 Most RTS options are disabled. Link with -rtsopts to enable them.

Adding -rtsopts to ghc-options in the test-suite sections of silently.cabal did not help.

However, perhaps we should also add a test in the repo so that it could be verified in CI.

Yes, please!

[ruifengx]

Adding -rtsopts to ghc-options in the test-suite sections of silently.cabal did not help.

Indeed this is very confusing. I am using stack, but I guess it is the same problem with cabal. I needed to run stack clean before the -rtsopts option takes any effect in the resulting executable.

Regarding the test, I did not touch it because I was a bit confused. I don't see the difference between the current two test suites. It looks to me that they are using the same source files, but in different manners (by depending on the library or directly adding the relevant module as part of the test). (From the commit history I see those two once were different, but it seems later commits made them essentially the same.)

Another thing to note: the implementation in this PR cannot deal with file handles, so the following entry in the current test-suite actually fails:

silently/test/Spec.hs

Lines 18 to 26 in 1545584

	describe "hSilence" $ do
	it "prevents output to a given handle" $ let file = "foo.txt" in do
	h <- openFile file ReadWriteMode
	hSilence [h] $ do
	hPutStrLn h "foo bar baz"
	hFlush h
	hSeek h AbsoluteSeek 0
	hGetContents h `shouldReturn` ""
	`finally` removeFile file

Therefore this one should be made conditional. It should be disabled for the combination GHC 9+, Windows, and --io-manager=native. Currently the CI does not involve testing on Windows; I will look into the configuration to see what is the best way to add this combination.

[andreasabel]

Thanks for the hint with clean! Now I can run the test:

$  cabal test --test-options='+RTS' --test-options='--io-manager=native' --test-show-details=direct
...
Test suite spec-generic: RUNNING...
/hSilence/prevents output to a given handle/ FAILED
*** Exception: C:\Users\admin\AppData\Local\Temp\silence-78634d9b-a1d3-4e7b-ac77-788b698cc70b: hDuplicateTo: unsupported operation (Operation is not supported)
/capture/captures stdout/ FAILED
*** Exception: C:\Users\admin\AppData\Local\Temp\capture-78718886-1b67-4654-a089-5198fee1cb53: hDuplicateTo: unsupported operation (Operation is not supported)
/hCapture/preserves NoBuffering/ FAILED
*** Exception: C:\Users\admin\AppData\Local\Temp\capture-457bf622-2c17-483a-9589-049cd93a1749: hDuplicateTo: unsupported operation (Operation is not supported)
/hCapture/preserves LineBuffering/ FAILED
*** Exception: C:\Users\admin\AppData\Local\Temp\capture-fe4f6b30-5ae2-4e38-bcf0-204049d45171: hDuplicateTo: unsupported operation (Operation is not supported)
/hCapture/preserves BlockBuffering Nothing/ FAILED
*** Exception: C:\Users\admin\AppData\Local\Temp\capture-3b307c48-9bc6-429b-9ff0-12c308140a93: hDuplicateTo: unsupported operation (Operation is not supported)
5 example(s), 5 failure(s)
Test suite spec-generic: FAIL

[ruifengx]

Sorry, it turns out that my implementation does not actually prevent writing to stdout/stderr. Last time I apparently was testing the wrong thing. Now I believe it is in fact impossible for the functions here to work correctly, if the native IO manager is used on Windows. My approach would only work if every write to the stdout/stderr uses GetStdHandle to fetch the handle, but in the actual implementation, the handles are cached. There is no known API to change the underlying content of any handle, and the cached handles themselves cannot change, so I think we are basically stuck here.

Now the only thing this PR does is to prevent crash from the unsupported hDuplicateTo. I, as a user, would still prefer no crash in e.g. stack, so I still hope this could get merged. But I understand it if you think otherwise. Anyway, I believe the proper fix should be eventually done in downstream libraries and programs.

EDIT: I see the log you posted, and tried using cabal. However, I did not get the exceptions:

$ cabal test --test-options='+RTS' --test-options='--io-manager=native'
/hSilence/prevents output to a given handle/ FAILED
expected: ""
 but got: "foo bar baz\n"
/capture/captures stdout/ fooFAILED
expected: ("foo",23)
 but got: ("",23)
/hCapture/preserves NoBuffering/ OK
/hCapture/preserves LineBuffering/ OK
/hCapture/preserves BlockBuffering Nothing/ OK
5 example(s), 2 failure(s)

And please feel free to close this PR if you prefer making the error explicit and have downstream programs fix the real problem.

[andreasabel]

In general, I prefer crashes over error-swallowing. So atm I am not inclined to merge this.
Also, shouldn't the io-manager be queried to decide whether the original behavior is ok or whether the workaround needs to kick in?

Thanks for your investigation! I linked you PR upstream: https://gitlab.haskell.org/ghc/ghc/-/issues/22146#note_454605

[ruifengx]

In general, I prefer crashes over error-swallowing.

Understood. It turns out that the native IO manager might also have other bugs (see for example commercialhaskell/stack#5851 (comment)), so the problem is not stuck here anyway.

Also, shouldn't the io-manager be queried to decide whether the original behavior is ok or whether the workaround needs to kick in?

Actually it is queried. The import of (<!>) from GHC.IO.SubSystem (line 31) is for the query. The expression f <!> g returns f if the IO manager is POSIX, and g otherwise.

[andreasabel]

Actually it is queried. The import of (<!>) from GHC.IO.SubSystem (line 31) is for the query. The expression f <!> g returns f if the IO manager is POSIX, and g otherwise.

Ah, thanks for the explanation!