Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

io free monad #881

Merged
merged 1 commit into from
Sep 15, 2017
Merged

io free monad #881

merged 1 commit into from
Sep 15, 2017

Conversation

fwbrasil
Copy link
Collaborator

@fwbrasil fwbrasil commented Sep 11, 2017

Fixes #21

Problem

Quill performs side effects but doesn't provide an easy way to isolate them.

Solution

Introduce the IO free monad that defers execution until unsafePerformIO is called.

Notes

The execution of IO uses the same execution mechanism as its context. For instance, it doesn't try to implement an async wrapper on top of jdbc; the return type of a synchronous context is synchronous.

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@fwbrasil
Copy link
Collaborator Author

@getquill/maintainers this is ready for review. The readme has more information.

@missingfaktor I'd love to hear your thoughts on this if you have some time to spare.

@fwbrasil fwbrasil changed the title [WIP] io free monad io free monad Sep 12, 2017
@mosyp
Copy link
Collaborator

mosyp commented Sep 13, 2017

Great work!
However it seems like you need context instance to create io task and then execute them using those contexts, right? Hence particular io task is bound to context.
Is it possible to separate this? Something like this:

val task = io(query[Person].insert(lift(p))).flatMap { _ =>
   io(query[Person])
}
ctx.run(io)

Where ctx could be any of supported, future, twitter future, raw sync etc., and io is a free monad

@mosyp
Copy link
Collaborator

mosyp commented Sep 13, 2017

I was talking about something like this

val sql = new SqlDialect[SnakeCase] // It could be even static since it is stateless
val io = sql.run(query[Person])
  .filter(_.age > 10)
  .flatMap(p => sql.run(query[Person].delete(lift(p))))

val asyncCtx = new AsyncPostgresContext
val syncCtx = new SyncMySqlContext

asyncCtx.run(io) // res1: Future[Unit]
syncCtx.run(io) // res2: Unit

So we have 2 separate layers, free monad for query generation and second one for actual execution

@fwbrasil
Copy link
Collaborator Author

@mentegy thanks for the feedback. That's a good point, but there are more complexities to it. We made the decision of making everything dependent on the context some time ago (#417). I think we could revisit this decision before 2.0, but it's definitely something that shouldn't be in the scope of this change given the complexity. Could you open a new issue with your proposal so we can discuss in detail?

@mosyp
Copy link
Collaborator

mosyp commented Sep 13, 2017

@fwbrasil I got this, description of #21 confused me a little bit. Everything else looks great for me except unsafePerformIO naming. I think that performIO is enough or something like this, hence, it's actually safe (for futures at least, depends on context) and there's no alternative such as safePerformIO

@fwbrasil
Copy link
Collaborator Author

@mentegy the "unsafe" part is a convention to indicate that the method performs side effects. I agree that it can be confusing. I'll rename it.

case Sequence(in, cbfIOToResult, cbfResultToValue) =>
val builder = cbfIOToResult()
in.foreach(builder += unsafePerformIO(_))
Future.sequence(builder.result)(cbfResultToValue, ec)
Copy link
Collaborator

@jilen jilen Sep 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this leads postgres-async and mysql-async to run multi query at the same time on same connection ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jilen yes. Do you ask that because of the concurrency issue with parallel queries under a transaction? I think that's a driver bug. For instance, ndbc handles this scenario without problems.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwbrasil Yes. And if it not support sequence we could point it out somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jilen I think that's a separate issue. The same happens with Future.sequence. Could you make sure that we have an issue open for it? If the driver doesn't fix it, we can workaround on the quill side by having a synchronization point for transactions.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fwbrasil I'll open an issue while this pr merged

@fwbrasil
Copy link
Collaborator Author

@mentegy unsafePerformIO renamed to performIO

Copy link
Collaborator

@mosyp mosyp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants