build failure: conduit 1.3.0 deprecated addCleanup

[guibou]

Conduit addCleanup function was removed in conduit-1.3.0, as discussed here:

• https://www.snoyman.com/blog/2018/01/drop-conduits-finalizers
• https://www.snoyman.com/blog/2018/02/conduitpocalypse

However bindings-gdal still use them in GDAL/Internal/Layer.chs https://github.com/albertov/bindings-gdal/blob/master/src/GDAL/Internal/Layer.chs#L380

[albertov]

Hi, thanks for reporting this. I haven't had a chance to upgrade to conduit 1.3 yet so I didn't notice.

Is there any way to achieve similar functionality (ie: have OGR transactions rolled back automatically in case of exception inside the GDAL scope) with the new conduit? Otherwise I guess the API will have to break since I can't think of other way of implementing it except by making it continuation-based (like bracket)

[guibou]

@albertov actually I'm not used to conduit at all, so I cannot answer your question ;( I'm just reporting it and for now I'll use conduit 1.2 in my project.

[guibou]

@albertov how can I test the correct behavior of the patch I'm working on?

Actually, if I just totally remove the addCleanup and catchC, the testsuite does pass! And this feels weird, because I did not even call the commit function. Do you think you can write a test which fails without the addCleanup call, so that I can work with a safety net? ;)

From the current implementation, I understood this behavior:

• inside seed pipeline is executed
• In case of exception in the pipeline, rollback is executed
• When the pipeline is terminated and if it goes to completion, commit is executed.

I'll be tempted to apply this patch:

diff --git a/src/GDAL/Internal/Layer.chs b/src/GDAL/Internal/Layer.chs
index e435b47..944f0c5 100644
--- a/src/GDAL/Internal/Layer.chs
+++ b/src/GDAL/Internal/Layer.chs
@@ -78,14 +78,14 @@ module GDAL.Internal.Layer (
 import Data.ByteString.Unsafe (unsafeUseAsCString)
 import Data.Coerce (coerce)
 import Data.Conduit ( Conduit, Sink, Source
-                    , addCleanup, awaitForever, yield
+                    , awaitForever, yield
                     , bracketP, catchC
                     , (=$=))
 import qualified Data.Conduit.List as CL
 import Data.Text (Text)
 
 import Control.Applicative (Applicative, (<$>), (<*>), pure)
-import Control.Exception (SomeException)
+import Control.Exception (SomeException(..))
 import Control.Monad (liftM, when, void, (>=>), (<=<))
 import Control.Monad.Base (MonadBase)
 import Control.Monad.Catch (
@@ -377,9 +377,12 @@ instance HasLayerTransaction ReadWrite where
     bracketP (alloc' state) free $ \ seed@(layer,_) -> do
       liftIO $ checkOGRError "StartTransaction" $
         {#call OGR_L_StartTransaction as ^#} (unLayer layer)
-      addCleanup (\terminated -> when terminated (commit layer)) $
+
+      res <-
         inside seed `catchC`
           \(e :: SomeException) -> rollback layer >> throwM e
+      commit layer
+      pure res
     where
       alloc' = runWithInternalState alloc
       free = closeLayer . fst

• in case of exception in the inside seed pipeline, catchC will rollback the transaction and propagate the exception to the upper level which will call bracketP finalizer, as excepted.
• in case of no exception, commit layer is called and the result is returned.

The only semantic possible difference I see with the previous implementation with addCleanup is that addCleanup was able to detect if the pipe was "early terminated" and was able to avoid the call to commit if the pipe was early terminated. My understanding of "early termination" is that the pipe ends with an exception, hence the behavior is respected.

Does it exists another source of "early" termination?

Note that with some others mechanical changes, I'm able to run bindings-gdal with conduit-1.3.

[albertov]

Hi @guibou . Sorry for the extremely late reply. I've missed your post until now. It's very possible that there's no test for this behavior so that would explain why removing addCleanup still makes the test suite pass. The patch you propose looks good. It seems to achieve the "rollback on exception" functionality in a less convoluted way than before. Do you have this fix and the conduit upgrade available somewhere? I'd be very happy to merge a PR. Thanks

[guibou]

@albertov the addCleanup removal PR is merged.

I'll push the conduit upgrade soon. Do you care about backward compatibility with previous conduit version or not?

[albertov]

@guibou thanks! I don't personally care since I pin the version with nix. If it's compatible with both then great, else no big problem.

[guibou]

Actually I was asking the question because I need to update the nix file too ;) Do you care about ghc 8.0 support or can I just tell the nix file to use whatever ghc is currently the default in nix?

[albertov]

Better use the default one so it can fetch pre-compiled packages from cache.nixos.org, else there's a high chance it'll timeout in travis. 8.0 is outside the 2 year support period so we can ignore it at this point