Confine SqlCursor to the critical section accessing the connection. #2132
Conversation
bce1d15 to
f4f0518
Compare
|
Conceptually I like this. The execute is similar in concept as it just takes the bindings as a lambda and returns nothing. It would make the driver internal management somewhat simpler. However, as an alternative to solve #2123, if we wanted to keep the API we could keep the db connection aligned to the thread until everything is complete. That would mean the user needs to be careful to properly close the cursor, but you could argue that's true now. Will comment on the "close cursor" in the issue. |
|
on first blush i like it. it will be a bit (likely on the order of a couple months) before we're ready to do an API-breaking release of sqldelight so at that time we can get this merged in. |
|
Cool, I will proceed to update the tests. 👍 |
2850495 to
b0848b2
Compare
|
Some changes made:
|
0785e86 to
96c453e
Compare
5086701 to
a27dd34
Compare
a27dd34 to
7cab47d
Compare
fff878c to
2b50d14
Compare
7800a77 to
fe8b74a
Compare
|
We're now in API breaking territory, I re-reviewed the API changes and they still look good to me, are you able to rebase this one? If not I can give it a go at some point |
|
@AlecStrong mind if I take a crack at rebasing? |
|
👍 all yours, package name is probably the most painful one |
|
I don't think I can push here, so I pushed to https://github.com/eygraber/sqldelight/tree/fix-escaped-cursor |
|
ya just open a new pr so we can get ci going |
|
Merged here |
This is a proposed solution to fix the read-your-writes consistency violation as described in the issue:
It is API breaking however, since it changes all method signatures that previously return a
SqlCursor.TL;DR
SqlCursoris no longer returned to the caller.Instead, the caller must supply a
(SqlCursor) -> Rlambda that uses the cursor, which the generic lambda resultRwill be passed back to the caller.The above API changes allow the driver to close the
SqlCursorunconditionally, after invoking the supplied lambda. Most importantly, it can do so before the critical section on the connection ends.This guarantees that all active select statements are always closed, before the connection is reused by another caller/thread. Only when there is no active statement, the connection can then move its end mark forward for future reads to see newly written data.
Note that these changes are applicable to SQLite on other platforms as well.
Note
This is API breaking in both
SqlDriverandQuery<T>. Especially the latter, which exposes to library users aexecute()returning aSqlCursorthat can be closed asynchronously (or never).Though it seems to me that
executeAs*()are the ones intended to be used, notexecute(). So the proposal here is to replacefun execute(): SqlCursorwithout deprecation.