backend-db: Actually catch serialization errors to retry them #131
Conversation
3noch
force-pushed
the
eac@fix-serializable-retry
branch
from
b322cfa
to
355e222
Compare
| @@ -42,7 +45,7 @@ import qualified Control.Monad.State as S | |||
| -- It "disallows" (makes harder) arbitrary IO. | |||
| -- It "disallows" (makes harder) catching IO exceptions *inside* the transaction. | |||
| newtype Serializable a = Serializable (ReaderT Pg.Connection (LoggingT IO) a) | |||
| deriving (Functor, Applicative, Monad, MonadThrow, MonadLogger) | |||
| deriving (Functor, Applicative, Monad, MonadCatch.MonadThrow, MonadLogger) | |||
| -- NOTE: We *intentionally* leave out | |||
| -- - 'MonadCatch' so you can't accidentally mask a serialization error from the outer retry logic. | |||
There was a problem hiding this comment.
haha that's confusing.... I'll import MonadThrow unqualified.
There was a problem hiding this comment.
Rather than leaving out the typeclass, wouldn't it be better to provide a custom implementation that gives the behavior we want?
There was a problem hiding this comment.
What behavior would that be? A catch that just catches SqlSerializationError first and rethrows it before running your handler? I suppose that would ensure this error is never masked.
There was a problem hiding this comment.
I think that wouldn't be technically law-abiding
catch (throwM e) f = f ePresumably this law is for all e and f. If we "unmasked" SqlSerializationError in catch then any f :: SqlSerializationError -> m a would not result in f e but an exception.
How much do we care...
There was a problem hiding this comment.
FTR:
instance MonadCatch Serializable where
catch m f = MonadCatch.catches m
[ MonadCatch.Handler $ \(e :: SqlSerializationError) -> MonadCatch.throwM e -- Never let a handler mask this exception
, MonadCatch.Handler f
]There was a problem hiding this comment.
we could make it (kinda) law-abiding again by also changing our throwM i think. it would be tricky and almost certainly not worthwhile for what it actually buys. I'm just not sure we actually want MonadCatch here though, particularly since also any other SqlError could cause the transaction to abort and would need to be handled specially if you don't abort the Serializable action also ...
3noch
force-pushed
the
eac@fix-serializable-retry
branch
from
355e222
to
e0fd1f2
Compare
3noch
force-pushed
the
eac@fix-serializable-retry
branch
from
e0fd1f2
to
ee08d02
Compare
3noch
force-pushed
the
eac@fix-serializable-retry
branch
from
2c53fe5
to
026ff08
Compare
| -- | Like 'Pg.withTransactionModeRetry' but polymorphic over exception type. | ||
| -- Copied from https://github.com/phadej/postgresql-simple/blob/e02684f9c38acf736ac590b36b919000a2b45bc4/src/Database/PostgreSQL/Simple/Transaction.hs#L156-L174 | ||
| withTransactionModeRetry' :: forall e a. E.Exception e => Pg.TransactionMode -> (e -> Bool) -> Pg.Connection -> IO a -> IO a | ||
| withTransactionModeRetry' mode shouldRetry conn act = |
There was a problem hiding this comment.
It's probably not worth copying this impl just to get the polymorphic exception handling.
There was a problem hiding this comment.
Nevermind, upstream uses SqlError. But I could upstream this version.
There was a problem hiding this comment.
There was a problem hiding this comment.
This has been merged. So we can point at original repo now.
Todo: