Open guibou opened 6 years ago
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)
@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.
@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 executedrollback
is executedcommit
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
inside seed
pipeline, catchC
will rollback
the transaction and propagate the exception to the upper level which will call bracketP
finalizer, as excepted.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
.
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
@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?
@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.
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?
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
Conduit
addCleanup
function was removed inconduit-1.3.0
, as discussed here: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