Enable transactional endpoint evaluation. #700

BearLemma · 2022-01-26T11:43:29Z

No description provided.

cli/tests/lit/transaction.lit

server/src/deno.rs

server/src/query/engine.rs

glommer · 2022-01-26T14:06:45Z

server/src/query/engine.rs

-        let mut borrow = self.stream.borrow_mut();
-        borrow.as_mut().unwrap().as_mut().poll_next(cx)
+        let mut lock_borrow = self.lock.borrow_mut();
+        let mut stream_borrow = self.stream.borrow_mut();


this needs a shitload of comments.

I've added some comments from my limited POV. I don't think I understand it fully though. The code is crazy complicated and it should burn.

glommer · 2022-01-26T14:09:40Z

It's good, but the unsafe blocks need a lot of comments explaining why they are safe, and potentially some asserts to make sure the pointers are not null and obey preconditions we may want.

espindola · 2022-01-26T19:40:39Z

This should probably be rebased -i. For example, there is no reason to define TransactionStatic as Rc and then change it to Arc.

espindola · 2022-01-26T19:41:39Z

Also, we should not have a commit message "fix" :-) I did that to have a single commit on top of the old branch, those changes should be split among other commits in a meaningful way.

glommer · 2022-01-26T19:43:48Z

likely a squash is enough here

espindola · 2022-01-26T20:58:57Z

likely a squash is enough here

+1

espindola · 2022-01-26T21:31:21Z

server/src/query/engine.rs

+pub(crate) type TransactionStatic = Arc<Mutex<Transaction<'static, Any>>>;
+
+struct QueryResults<T> {
+    fut: RefCell<Option<T>>,


Having a single inner: RefCell is a bit cleaner, but not much.

espindola · 2022-01-26T22:16:26Z

If smol-rs/async-lock#20 is approved it would simplify this code a bit.

BearLemma · 2022-01-27T09:53:57Z

I fixuped the fix commit.

espindola · 2022-01-27T14:09:55Z

I just noticed that tokio's mutex has an easier to use API:

pub async fn lock_owned(self: Arc) -> OwnedMutexGuard

which is somewhat on the lines of the patch I sent to async_mutex. Using that could simplify this code, but add more dependencies on tokio.

BearLemma · 2022-01-27T14:12:02Z

I just noticed that tokio's mutex has an easier to use API:

pub async fn lock_owned(self: Arc) -> OwnedMutexGuard

which is somewhat on the lines of the patch I sent to async_mutex. Using that could simplify this code, but add more dependencies on tokio.

@espindola would you mind suggesting a patch? I'm really afraid to touch that code. I think that any simplification would be very appreciated in this case. Regarding Tokio dependencies - is it a problem?

espindola · 2022-01-27T14:28:55Z

Please do squash or otherwise reorganize the commits. We still have a patch with:

-pub(crate) type TransactionStatic = Rc<Mutex<Transaction<'static, Any>>>;
+pub(crate) type TransactionStatic = Arc<Mutex<Transaction<'static, Any>>>;

Also, tests fail in the intermediate commits.

espindola · 2022-01-27T14:29:51Z

@espindola would you mind suggesting a patch? I'm really afraid to touch that code. I think that any simplification would be very appreciated in this case. Regarding Tokio dependencies - is it a problem?

I will try. Not sure what the status is for trying to not use tokio, but it is always nice to have options.

BearLemma · 2022-01-27T14:32:57Z

Please do squash or otherwise reorganize the commits. We still have a patch with:
-pub(crate) type TransactionStatic = Rc<Mutex<Transaction<'static, Any>>>;
+pub(crate) type TransactionStatic = Arc<Mutex<Transaction<'static, Any>>>;
Also, tests fail in the intermediate commits.

Squashed everything.

espindola · 2022-01-27T15:05:56Z

I have pushed a496bb3. Feel free to include it if you like it.

BearLemma · 2022-01-27T19:09:36Z

I have pushed a496bb3. Feel free to include it if you like it.

@espindola very nice, I squashed it in.

espindola · 2022-01-27T20:05:19Z

@espindola very nice, I squashed it in.

And I have now pushed d2a3731, which uses pin_project. The net result is that we get only one extra unsafe with this PR if that is included.

espindola · 2022-01-27T20:19:12Z

@espindola very nice, I squashed it in.

And I have now pushed d2a3731, which uses pin_project. The net result is that we get only one extra unsafe with this PR if that is included.

And d3dc67f on top of that that uses flatten_stream.

BearLemma · 2022-01-27T20:31:38Z

@espindola very nice, I squashed it in.

And I have now pushed d2a3731, which uses pin_project. The net result is that we get only one extra unsafe with this PR if that is included.

@espindola doesn't this part of the code use a pointer to a local variable (the query_string seems to be just a local string)? That doesn't look very safe at all 🤔

espindola · 2022-01-27T20:55:15Z

@espindola very nice, I squashed it in.

And I have now pushed d2a3731, which uses pin_project. The net result is that we get only one extra unsafe with this PR if that is included.

@espindola doesn't this part of the code use a pointer to a local variable (the query_string seems to be just a local string)? That doesn't look very safe at all thinking

It doesn't use a pointer to the String, it uses a pointer to the str.

espindola · 2022-01-27T21:38:40Z

Take a look at 95e299f. It is rebased on top of #725. It adds only one unsafe block, which is required because of the sqlx api.

espindola · 2022-01-27T22:46:04Z

Take a look at 95e299f. It is rebased on top of #725. It adds only one unsafe block, which is required because of the sqlx api.

rebased espindola/jan again. it is now 081de5d

BearLemma · 2022-01-28T13:20:01Z

Take a look at 95e299f. It is rebased on top of #725. It adds only one unsafe block, which is required because of the sqlx api.

rebased espindola/jan again. it is now 081de5d

Oh man, that looks so much better than the original. I can actually understand that. Awesome work.
So I've pushed your work to this branch and I suppose we can merge it? @glommer ?

espindola · 2022-01-28T13:29:28Z

BTW, smol-rs/async-lock#20 got merged, so once that is released we should be able to move back to async_mutex.

glommer · 2022-01-28T13:34:45Z

docusaurus/docs/data-access.md

@@ -212,7 +212,7 @@ The `findMany()` method is convenient, but also problematic if you have a lot of
 entities stored because loading them can take a lot of time and memory. In future
 releases of ChiselStrike, the runtime will enforce a maximum number of entities
 `findMany()` can return and also enforce timeouts at the data store level. The
-runtime will also provide optional pagination for the `findMany()` method. 
+runtime will also provide optional pagination for the `findMany()` method.


for the future: avoid unrelated changes to the PR

Yeah but this is an editor extension that trims trailing white-spaces on save...

that's fine, but use git add -p to select what goes into the PR or not

glommer · 2022-01-28T13:36:26Z

docusaurus/docs/data-access.md

+transaction automatically.
+


Follow up: Mention that the user should be careful that external accesses like POST requests with side effects obviously are not part of the transaction and users should be careful with that

I don't understand the situation. You mean that our user would call a chisel endpoint within another chisel endpoint? Doesn't that go without saying?

docusaurus/docs/known_issues.md

Fix transactions test.

espindola · 2022-01-28T14:14:20Z

So, is this approved? Looks like github somehow lost the approval.

BearLemma · 2022-01-28T14:15:47Z

So, is this approved? Looks like github somehow lost the approval.

I had to do another rebase as there were changes in main and approval is lost when you push a change to the branch upon which a PR is based.

espindola

reapproving