Add Acquire based interfaces for connections. by merijn · Pull Request #984 · yesodweb/persistent

merijn · 2019-11-06T16:31:11Z

Add Acquire based interface for starting transactions and getting
connections from a Pool for use in MonadResource instances that are not
MonadUnliftIO, such as Conduit.

Before submitting your PR, check that you've:

Bumped the version number
Documented new APIs with Haddock markup
Added @since declarations to the Haddock

After submitting your PR:

Update the Changelog.md file with a link to your PR
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

merijn · 2019-11-06T17:33:01Z

The LTS-9 looks to be due to a different with being in scope/resourcet having a different type in that version?

I don't get the MySQL error in nightly, seems to be unrelated?

merijn · 2019-11-06T21:50:04Z

LTS-9 failure is indeed due to ResourceT not being based on MonadUnliftIO in that version, not really sure how we could work around that.

MySQL error in nightly appears to be a parse error in timestamps (which doesn't seem remotely related to anything I changed).

parsonsmatt · 2019-11-06T22:15:30Z

MySQL error in nightly appears to be a parse error in timestamps (which doesn't seem remotely related to anything I changed).

I suspect that it's because the instance Read UTCTime became more strict, requiring the Z suffix. I had to fix similar errors in esqueleto recently.

LTS-9 failure is indeed due to ResourceT not being based on MonadUnliftIO in that version, not really sure how we could work around that.

lts-9 is GHC 8.0.2, which is out of the "support 3 major versions" release policy. I'd be OK with dropping support for this.

A compatibility module that had some CPP and used MonadBaseControl.. would be disgusting but could work.

merijn · 2019-11-06T23:03:33Z

A compatibility module that had some CPP and used MonadBaseControl.. would be disgusting but could work.

Not sure that'd even work, because this is part of the visible persistent API, since there's no way to guarantee that every MonadBaseControl IO instance has a corresponding MonadUnliftIO and vice versa, afaict.

merijn · 2019-11-27T00:05:03Z

@parsonsmatt What would dropping lts-9 support entail, just deleting the lts-9 line from .travis.yml?

I'd like to see this and #983 merged and making it onto Hackage (although, since I'd also like to bump the SQLite version shipped, perhaps a new Hackage release should wait until I open a PR for that).

merijn · 2019-12-15T21:41:41Z

@parsonsmatt Ping?

I'm not really sure what dropping lts-9 support entails beyond deleting the tests from .travis.yml, additionally I'd (ideally) like to see #983 and #991 merged and released before the busy holiday season starts :)

parsonsmatt

This is great. Thanks!

parsonsmatt · 2019-12-28T16:58:43Z

persistent/Database/Persist/Sql/Run.hs

+-- on a connection that cannot be done within a transaction, such as VACUUM in
+-- Sqlite.
+--
+-- @since 2.10.5


parsonsmatt · 2019-12-28T17:00:31Z

persistent/Database/Persist/Sql/Run.hs

    :: (MonadUnliftIO m, BackendCompatible SqlBackend backend)
    => ReaderT backend m a -> Pool backend -> m a
-runSqlPool r pconn = withRunInIO $ \run -> withResource pconn $ run . runSqlConn r
+runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runReaderT r


The change from runSqlConn to runReaderT drops the logic around transactions and exception handling. Why'd this get changed?

Suggested change

runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runReaderT r

runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runSqlConn r

parsonsmatt · 2019-12-28T17:01:29Z

persistent/Database/Persist/Sql/Run.hs

    => ReaderT backend m a -> Pool backend -> IsolationLevel -> m a
-runSqlPoolWithIsolation r pconn i = withRunInIO $ \run -> withResource pconn $ run . (\conn -> runSqlConnWithIsolation r conn i)
+runSqlPoolWithIsolation r pconn i =
+    with (acquireSqlConnFromPoolWithIsolation i pconn) $ runReaderT r


Same here - runSqlConn does all the transaction stuff, so runReaderT here voids that safety. (Man I really need to newtype SqlPersistT ...)

Suggested change

with (acquireSqlConnFromPoolWithIsolation i pconn) $ runReaderT r

with (acquireSqlConnFromPoolWithIsolation i pconn) $ \conn -> runSqlConnWithIsolation r conn i

Wait, no, I think I see what's going on here. You've moved that responsibility into the Acquire interface. So runSqlConn has it's own logic for handling exceptions, and that logic is now sort of duplicated into the Acquire interface.

This is fine. Ignore the suggestions. I've reviewed the relevant code and as far as I can tell the behavior will be identical.

parsonsmatt · 2019-12-28T17:06:53Z

persistent/Database/Persist/Sql/Run.hs

-            (restore $ connRollback conn' getter)
-    restore $ connCommit conn' getter
-    return x
+runSqlConn r conn = with (acquireSqlConn conn) $ runReaderT r


😂 teaches me to review a PR before at least skimming it fully

parsonsmatt · 2020-01-03T19:43:10Z

@merijn OK, we've dropped support for lts-9 in the latest master - can you merge that in and get CI green? Then we can get this + your other SQLite PR out the door.

Thanks!

Add Acquire based interface for starting transactions and getting connections from a Pool for use in MonadResource instances that are not MonadUnliftIO, such as Conduit.

Turns out it's needed to do somethings using SQLite and there's no point in not exporting it and making users reimplement it themselves.

merijn · 2020-01-06T13:36:47Z

@parsonsmatt hmmm, mongodb seems to fail for some reason?

merijn · 2020-01-06T13:41:57Z

Which seems weird, as it works in your master and the failure seems to be caused by MonadFail, which I certainly didn't touch...

parsonsmatt · 2020-01-06T16:46:20Z

yeah the mongodb library probably just changed and since we don't have an upper bound it is going bonkers.

merijn · 2020-01-07T08:06:41Z

yeah the mongodb library probably just changed and since we don't have an upper bound it is going bonkers.

I can make another PR with an upperbound on mongodb to get the tests passing and rebase on top of that...

parsonsmatt · 2020-01-28T16:42:33Z

Released in persistent-2.10.5, thanks so much! 😄

merijn mentioned this pull request Nov 6, 2019

Absolute lack of thread-safety SqlBackend should be documented. #981

Open

parsonsmatt approved these changes Dec 28, 2019

View reviewed changes

parsonsmatt added the enhancement label Dec 28, 2019

merijn added 3 commits January 6, 2020 13:42

Add Acquire based interfaces for connections.

2fc94c0

Add Acquire based interface for starting transactions and getting connections from a Pool for use in MonadResource instances that are not MonadUnliftIO, such as Conduit.

Add ChangeLog entry.

f79eb87

Also export unsafeAcquireConnFromPool

341c3a8

Turns out it's needed to do somethings using SQLite and there's no point in not exporting it and making users reimplement it themselves.

merijn force-pushed the persistent-acquire branch from ed67e8a to 341c3a8 Compare January 6, 2020 12:43

parsonsmatt added this to the 2.10.5 milestone Jan 28, 2020

parsonsmatt merged commit 01b1269 into yesodweb:master Jan 28, 2020

merijn deleted the persistent-acquire branch February 11, 2020 09:48

	runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runReaderT r
	runSqlPool r pconn = with (acquireSqlConnFromPool pconn) $ runSqlConn r

	with (acquireSqlConnFromPoolWithIsolation i pconn) $ runReaderT r
	with (acquireSqlConnFromPoolWithIsolation i pconn) $ \conn -> runSqlConnWithIsolation r conn i

Conversation

merijn commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merijn commented Nov 6, 2019

Uh oh!

merijn commented Nov 6, 2019

Uh oh!

parsonsmatt commented Nov 6, 2019

Uh oh!

merijn commented Nov 6, 2019

Uh oh!

merijn commented Nov 27, 2019

Uh oh!

merijn commented Dec 15, 2019

Uh oh!

parsonsmatt left a comment

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

parsonsmatt Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

parsonsmatt commented Jan 3, 2020

Uh oh!

merijn commented Jan 6, 2020

Uh oh!

merijn commented Jan 6, 2020

Uh oh!

parsonsmatt commented Jan 6, 2020

Uh oh!

merijn commented Jan 7, 2020

Uh oh!

parsonsmatt commented Jan 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

merijn commented Nov 6, 2019 •

edited

Loading