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

Add support for async drivers #3168

Merged
merged 34 commits into from
May 5, 2022
Merged

Conversation

dellisd
Copy link
Collaborator

@dellisd dellisd commented Apr 30, 2022

Regarding #1942

This adds the main codegen changes along with a minimal runtime needed to support async driver implementations, along with some basic (and incomplete) implementations of an R2DBC driver and a driver for running sql.js in a web worker. There's still a lot of work (and cleanup) to be done on it, but I'm hoping to get some feedback on the design of the runtime before going any further.

To summarize how it works, async code generation is enabled in the gradle config:

sqldelight {
  database("MyDatabase") {
    packageName = "com.test.db"
    generateAsync = true
  }
}

SQLdelight will then generate queries (and related code) that return an AsyncSqlDriver.Callback<R> object that can be used to observe the result/errors. This object can be easily adapted to other reactive/async frameworks, like these coroutine extensions.

Drivers implement the AsyncSqlDriver interface which is pretty similar to SqlDriver except most of the methods now return the callback object are now suspend functions.

Open Questions

  1. Is it worth implementing and maintaining this whole async runtime just to enable support for other frameworks? It adds a lot of complexity both in the runtime implementation and codegen compared to just generating and implementing suspend functions.
  2. What to do about transactions? From reading past issues, it looks like there could be trouble when dealing with multithreading? Also, transactionWithResult could become trouble since we would presumably want to be able to support the semantics of whatever reactive library is being used which currently isn't possible (related to Expose Transacter.Transaction components to be used from a SqlDriver #1856)
    e.g. for coroutines we'd want something like
val list = nameQueries.suspendingTransactionWithResult { 
  nameQueries.insert(/* ... */).await()
  nameQueries.getAll().awaitAsList()
}
  1. R2DBC returns results as a stream of rows, rather than a cursor that can be controlled directly and synchronously. This is incompatible with the existing SqlCursor interface, and my current workaround in the R2DBC driver implementation is to read all the rows into a map and then hand that to a SqlCursor implementation. Not ideal. Adapting all of the mapper code to follow the asynchronous semantics would require a lot more effort to change the codegen.
  2. This isn't really related to "async sqldelight" nor is it really a question, but the R2DBC implementations are weird. The h2 and postgres drivers require statement placeholders to use the $1, $2 format instead of ?, ? so they don't work with the current R2dbcDriver, only mysql works. Special sqldelight driver implementations could probably work around it? But I also don't know how much demand there is for R2DBC in sqldelight, I mainly implemented it just to help inform the design of the async runtime.

Lastly, I threw together a quick sample project that shows the sql.js worker driver in action: https://github.com/dellisd/sqldelight-js-worker

Add async drivers for R2DBC and sqljs workers
@AlecKazakova
Copy link
Collaborator

Have only done a skim, but this is heading in the right direction.

We should create a new module for the async runtime (async-runtime or something like that) that has all these new types, and potentially duplicates some other types like Schema that I assume we're reusing right now. I'd like for the two worlds to be pretty separate in terms of importing a public API when you switch to async. The gradle plugin adds the runtime dependency so it can pretty smoothly switch to the async runtime if needed. If there is a huge amount of common ground between the async and sync runtimes, we can create a common module or something for them to share.

  1. I don't think its worth maintaining. coroutines is the standard for async in kotlin now, I don't think it would be controversial to have it be what we output, and similarly if people want to convert to their own type it will be possible.
  2. In the same vein, I think we just expose suspending functions for transactions. I'm unfamiliar with the issues that'll come up because of that - but maybe theres a world where we just enforce that statements within the transaction happen within the same context, or something like that. I assume thats the problem where you can't have the different statements running on different threads since then they may use different database connections. If we record the context the transaction was started with maybe we can avoid that?
  3. Cool with that - since theres already a lot going on lets keep this initial implementation to fleshing out the API and then if we want to revisit things or make improvements we can do that in follow ups.
  4. No strong feelings here, the only async driver I've seen requested is jasync which has a coroutines compatible API.

@dellisd
Copy link
Collaborator Author

dellisd commented May 3, 2022

I've split the runtime module up to separate the async code and also extracted a few classes into a shared module that I don't think would make sense to duplicate (like ColumnAdapter).

There's also :runtime:internal which might be overkill for just a single function, but it's to prevent a shared internal function from being exposed to consumers. I can get rid of it if we don't want the whole extra module just for that, and I can also change the names since :runtime:runtime just looks silly

@AlecKazakova
Copy link
Collaborator

yep, module structure looks good

Update async runtime to use suspend functions
Add Closeable to async runtime
Update sqljs worker and r2dbc drivers
@AlecKazakova
Copy link
Collaborator

tests are failing because this code will need to be updated to publish the new runtime modules

@dellisd
Copy link
Collaborator Author

dellisd commented May 4, 2022

In terms of tests, many of the existing test suites can be duplicated and updated to check for suspend functions and Async* classes, but do you think it's worth duplicating every test? Since most of the codgen logic is untouched I figure it could be enough to add tests just to cover the query function signatures for now.

@AlecKazakova
Copy link
Collaborator

yea i wouldn't worry about duplicating all tests, its fine to just test that specific behavior once and rely on the normal tests for everything else

dellisd added 10 commits May 4, 2022 15:31
Add other async codegen compiler tests
Remove async extensions from coroutines-extensions (to revisit later)
Fix sample builds with new runtime modules
Add some brief documentation on implementing async drivers
@dellisd
Copy link
Collaborator Author

dellisd commented May 5, 2022

I think this now covers the core of what's needed for async drivers at least in terms of codegen and runtime API.

The sqljs worker driver is incomplete (lacks prepared statements and transactions) but I think it might be better to leave that as a separate PR since it'll be pretty involved on its own. The R2DBC driver isn't very polished and has limited actual driver support, and as mentioned earlier it's kind of unclear what the demand for it is so I'm not sure if we even want to keep it right now?

I'll add other small components like Flow extensions later (as a separate module from the existing extensions?)

But for now I think it's ready for some real reviews!

@dellisd dellisd marked this pull request as ready for review May 5, 2022 01:27
Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

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

Worked my way through the codegen, will review the runtime bits tomorrow

dellisd and others added 2 commits May 4, 2022 20:19
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
@AlecKazakova
Copy link
Collaborator

AlecKazakova commented May 5, 2022

As a side note (and potential nerd snipe) the jvm async driver I'm familiar with that appears to be getting traction is jasync which has built in support for coroutines so might integrate easier - and potentially it just already does the right thing™ for dates?

Add RuntimeTypes class for codegen types
Add default async runtime types
Remove suspend modifier from generated functions that return an `AsyncQuery`
@dellisd
Copy link
Collaborator Author

dellisd commented May 5, 2022

I didn't realize how tightly-coupled the driver-test module is to SQLite so the async version of it (which is basically just a copy) may not be all that useful in its current form. e.g. Postgres has no BLOB type and no changes() function.

@AlecKazakova
Copy link
Collaborator

yea lets just remove it, we can revisit the existing driver test stuff along with potentially adding an async one later

Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

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

looks good, fantastic stuff

}

private suspend fun <R> transactionWithWrapper(noEnclosing: Boolean, wrapperBody: suspend AsyncTransactionWrapper<R>.() -> R): R {
val transaction = driver.newTransaction()
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably we should figure out how to share this logic between the sync/async transacter types, but we can do that in a follow up

dellisd and others added 4 commits May 5, 2022 11:26
Co-authored-by: Alec Strong <AlecStrong@users.noreply.github.com>
Disable async codegen for HSQL
Remove unused functions in R2DBC driver
Move LogAsyncSqlDriver to tests
# Conflicts:
#	sqldelight-compiler/src/main/kotlin/app/cash/sqldelight/core/compiler/DatabaseGenerator.kt
@AlecKazakova AlecKazakova merged commit 371f503 into cashapp:master May 5, 2022
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.

None yet

2 participants