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

Abort unfinished transaction on Drop #7

Open
ShaddyDC opened this issue Nov 22, 2022 · 9 comments
Open

Abort unfinished transaction on Drop #7

ShaddyDC opened this issue Nov 22, 2022 · 9 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@ShaddyDC
Copy link

It would be convenient if a transaction that wasn't explicitly flushed with done() or commit() instead automatically called abort() when dropped.

This would allow users to more easily bail with the ? operator without leaving uncommited, half-finished transactions around.

@devashishdxt
Copy link
Owner

devashishdxt commented Nov 25, 2022

I agree with the intention but indexed db transaction behavior is dependent a lot on each browser's implementation.

Specification for abort() says that all the pending requests will be aborted (which does not mean all the requests made in that transaction). Depending on the browser implementation and user code, transaction may get auto committed even before calling commit(). I'm not sure if there's an easy way to handle this.

Worth reading: https://stackoverflow.com/questions/27326698/indexeddb-transaction-auto-commit-behavior-in-edge-cases

This will definitely require some testing/trial-and-error on each browser to see the behavior.

@ShaddyDC
Copy link
Author

I wasn't aware of that behaviour, but it does make sense. Thanks for the link.

If I understand the answers from a linked question correctly, transactions may commit as soon as the callbacks for all requests finish. Given the way the crate waits for each request to finish before returning control to a caller, I can basically never assume that 2 consecutive actions on an object store don't have a commit occur between them? That's unfortunate.

Without the ability to resume execution from the callbacks then, I have to wonder how useful it even is to expose the transaction layer to a user that can only make limited use of it. Though if it does usually work as expected, maybe that's reason enough. I haven't done any testing yet.

A possible, albeit hacky workaround would be to explicitly not return from the callbacks, until the next request is started or the transaction is otherwise explicitly finished by the user. That might go against the spirit of being a light wrapper, but it would keep the transaction alive, and dropping would work as expected. However, given that transactions should not be kept active while doing other works such as web requests, it might cause other issues for unsuspecting users.

I'm just spitballing. You've clearly got a much better idea than me about how the internals work to judge what appoach might work. :)

@devashishdxt
Copy link
Owner

@ShaddyDC If someone wants fine grained control over callbacks, they can always use idb-sys crate which exposes raw indexed db API without async/await.

@devashishdxt
Copy link
Owner

I have to wonder how useful it even is to expose the transaction layer to a user that can only make limited use of it.

Most of the times, it works as expected. There're also tests for testing abort() of transaction. But, because indexed db implementation can choose to auto-commit a transaction (because of various reasons like doing IO in event loop), it is unreliable and does not guarantee that the transaction will be aborted.

Also, I'm not sure how to handle abort failures if we call abort() in Drop implementation.

@ShaddyDC
Copy link
Author

That makes sense. Should I close the issue or would you prefer to keep it open in case you or anybody else has any good ideas for it in the future?

@devashishdxt
Copy link
Owner

I think eventually this needs to be resolved. We can keep it open for now.

@devashishdxt devashishdxt added enhancement New feature or request help wanted Extra attention is needed labels Dec 12, 2022
@Ekleog
Copy link

Ekleog commented Jan 12, 2024

Hmm… maybe an idea: what if Transaction were not a type, but a function that takes a Future?

The usage would then be something like (pseudo-code for clarity):

db.transaction(&["store"], ReadWrite, |transaction| async move {
    transaction.get("store", "key").await?;
    transaction.put("store", "key", "value").await?;
}).await?;

Then, the various transaction functions would:

  • submit a request to the IdbTransaction
  • make the success and error callbacks both poll the provided future exactly once, to let it make progress
  • assert that polling the provided future once results to either the future completing (thus committing the transaction), or a new request being submitted to transaction. If the assertion fails, it aborts the transaction before actually panicking

WDYT? This is still a bit hand-wavy, but I'm thinking it could make sense to have something like that :)

@Ekleog
Copy link

Ekleog commented Jan 14, 2024

Well, I gave it some more thought, and then decided to go ahead and implement it. It's now available at https://crates.io/crates/indexed-db ; though I have not implemented cursors yet — I'll first integrate it with my personal projects, check that the APIs are reasonably easy to use, and implement cursors after that :)

@Ekleog
Copy link

Ekleog commented Jan 14, 2024

I can confirm that the API of indexed-db (as of version 0.2.2, I had to make some changes after trying to use it a bit more) seems to work fine for me, and AFAICT prevents any invalid use of transaction that'd cause it to auto-commit.

Hope this helps :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants