Removing unliftio #1012

kazu-yamamoto · 2024-11-07T00:31:52Z

This PR removes unliftio from WAI families.
This is because TLS bye and network gracefulClose cannot used with bracket of unliftio.

In my field testing for a week, I have not found any problems.
This PR is just to run CI.

Vlix

Overall, this change seems to me as a bit too invasive.

Changing to Control.Exception will change behaviour with regards to async exceptions. If we'd want to keep it similar without unliftio, we'd need to use safe-exceptions.
I'd only fall back to Control.Exception if we make sure the masking stays the same and async exceptions are handled the same.

Unless anyone has a good argument (in every spot where catch, throwIO, bracket, etc. is used) as to why we don't have to.

Vlix · 2024-11-07T04:50:36Z

time-manager/System/TimeManager.hs

@@ -29,11 +29,11 @@ module System.TimeManager (
 ) where

 import Control.Concurrent (myThreadId)
+import qualified Control.Exception as E


I think the main change because of this would be that AsyncExceptions will also be ignored if they are thrown during timeoutAction (whereas they wouldn't before)

So (I think?) this means a thread might be failed to be killed if the async exception is thrown while the timeoutAction is running?
So we might want to change to safe-exceptions instead of unliftio?

Vlix · 2024-11-07T05:12:01Z

warp-tls/Network/Wai/Handler/WarpTLS.hs

-        putMVar var mct
-    mbs <- takeMVar var
+    let tm = settingsTimeout set * 1000000
+    mbs <- timeout tm recvFirstBS


I think this makes sense, but if we want to keep the same behaviour we'd have to change it to:

mbs <- unsafeUnmask $ timeout tm recvFirstBS

The original code before unsafeUnmask does not have unsafeUnmask.
Is it really necessary?

The umask in the diff is basically unsafeUnmask. But as you described earlier, that mkConn should be interruptable, this is unnecessary if the bracket (or whatever is used) is from Control.Exception instead of from UnliftIO.

Because the whole forkWithUnmask was a hack to get around the unliftio functions that mask more than necessary.

So no, I have changed my stance and adding unsafeUnmask is indeed not necessary.

Vlix · 2024-11-07T05:18:37Z

warp-tls/Network/Wai/Handler/WarpTLS.hs

@@ -366,7 +360,7 @@ httpOverTls TLSSettings{..} set s bs0 params =
        case mconn of
          Nothing -> throwIO IncompleteHeaders
          Just conn -> return conn
-    wrappedRecvN recvN n = handleAny (const mempty) $ recvN n
+    wrappedRecvN recvN n = handle (\(SomeException _) -> mempty) $ recvN n


Again, this will now also catch (and ignore) async exceptions.

Vlix · 2024-11-07T05:25:00Z

warp/Network/Wai/Handler/Warp/Conduit.hs

@@ -2,10 +2,10 @@

 module Network.Wai.Handler.Warp.Conduit where

+import Control.Exception (assert, throwIO)


This throwIO is fine if we're only throwing syncronous exceptions.

Vlix · 2024-11-07T05:28:00Z

warp/Network/Wai/Handler/Warp.hs

@@ -141,7 +141,7 @@ module Network.Wai.Handler.Warp (

 import Data.Streaming.Network (HostPreference)
 import qualified Data.Vault.Lazy as Vault
-import UnliftIO.Exception (SomeException, throwIO)
+import Control.Exception (SomeException, throwIO)


This throwIO is fine if we're only throwing syncronous exceptions.

Vlix · 2024-11-07T05:34:56Z

warp/Network/Wai/Handler/Warp/FdCache.hs

@@ -26,7 +26,7 @@ import System.Posix.IO (
    openFd,
    setFdOption,
 )
-import UnliftIO.Exception (bracket)
+import Control.Exception (bracket)


The UnliftIO bracket is different in that it uses uninterruptibleMask_ on the after actions. How important are the terminate actions?
They are cleaning up file descriptors, so that might be important enough to absolutely make sure it happens, right?

Resouce-release actions in bracket (i.e. masked actions) are iterruptible only if they are blocked (such as takeMVar).
Non-blocking actions are NOT iterruptible.
In other words, asynchronous exceptions are not delivered to resource-release actions if they are non-blocking.

terminate is non-blocking, so it's free from asynchronous exceptions, I believe.

Ah, yes. Never mind. I've re-read the bracket docs, and it does. I misread, I think.

Vlix · 2024-11-07T05:38:56Z

warp/Network/Wai/Handler/Warp/FileInfoCache.hs

@@ -7,14 +7,14 @@ module Network.Wai.Handler.Warp.FileInfoCache (
    getInfo, -- test purpose only
 ) where

+import Control.Exception (bracket, onException, throwIO)


UnliftIO.onException also runs with uninterruptibleMask. Making sure async exceptions wait until after the cleanup is done.

Vlix · 2024-11-07T05:44:39Z

warp/Network/Wai/Handler/Warp/Types.hs

@@ -7,7 +7,7 @@ module Network.Wai.Handler.Warp.Types where
 import qualified Data.ByteString as S
 import Data.IORef (IORef, newIORef, readIORef, writeIORef)
 import Data.Typeable (Typeable)
-import qualified UnliftIO
+import qualified Control.Exception as E


import Control.Exception as E (Exception, SomeException)

kazu-yamamoto · 2024-11-07T07:35:48Z

@Vlix Thank you for reviewing.
I don't want to use safe-exception as alternative unliftio.
This is because each library has its own semantics which I don't understand completely.
One example is timeout cannot be used in bracket of unliftio which suffered me very much.
I would like to use only Control.Exception since it is well-documented and I know it well.
Even we would adopt safe-exception, we cannot let all other libraries to introduce it.
The mixed situation is anxious.
I would like to set up a proper handler for Control.Exception even though it is boring.
I believe it saves my time In the long run.
I will check your comments above tomorrow.

Vlix · 2024-11-07T08:52:36Z

I would like to set up a proper handler for Control.Exception even though it is boring.

I can understand that.
I'd argue that we should try to mimic what unliftio did and change it in the situations where the unliftio style is not what we want.

kazu-yamamoto · 2024-11-07T22:45:01Z

I remembered one this.
So, I write this up just for record.

Some people believe that unliftio makes our libraries less-buggy.
But the reality is that unliftio can introduce new bugs.
An example is that unliftio made makeConn of warp-tls uninterruptible.
The fix was 371279f.

Vlix · 2024-11-08T07:27:11Z

Aaaah, that's why!

Ok, so what I think we should probably do:

identify the sections we want to be uninterruptible
identify which catches should let async exceptions through

Adding comments at those sections describing why we're using regular Control.Exception.catch or uninterruptibleMask or whatever would also be good for posterity.

kazu-yamamoto added 6 commits November 7, 2024 09:28

removing unliftio from time-manager

c68dc99

removing unliftio from warp-tls

0acc630

removing workaround for unliftio in warp-tls

155ad18

removing unliftio from warp

65c9669

removing unnecessary import

17ddfcc

using criterion instead of gauge and fixing bench

e991c3f

kazu-yamamoto merged commit 288e38f into yesodweb:master Nov 7, 2024
14 checks passed

kazu-yamamoto deleted the removing-unliftio branch November 7, 2024 01:49

Vlix reviewed Nov 7, 2024

View reviewed changes

kazu-yamamoto self-assigned this Nov 7, 2024

kazu-yamamoto mentioned this pull request Nov 13, 2024

Rethrow async exception #1013

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing unliftio #1012

Removing unliftio #1012

kazu-yamamoto commented Nov 7, 2024

Vlix left a comment

Vlix Nov 7, 2024

Vlix Nov 7, 2024

kazu-yamamoto Nov 13, 2024

Vlix Nov 13, 2024

Vlix Nov 7, 2024

Vlix Nov 7, 2024

Vlix Nov 7, 2024

Vlix Nov 7, 2024

kazu-yamamoto Nov 13, 2024

Vlix Nov 13, 2024

Vlix Nov 7, 2024

Vlix Nov 7, 2024

kazu-yamamoto commented Nov 7, 2024

Vlix commented Nov 7, 2024

kazu-yamamoto commented Nov 7, 2024

Vlix commented Nov 8, 2024

		@@ -2,10 +2,10 @@

		module Network.Wai.Handler.Warp.Conduit where

		import Control.Exception (assert, throwIO)

Removing unliftio #1012

Removing unliftio #1012

Conversation

kazu-yamamoto commented Nov 7, 2024

Vlix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazu-yamamoto commented Nov 7, 2024

Vlix commented Nov 7, 2024

kazu-yamamoto commented Nov 7, 2024

Vlix commented Nov 8, 2024