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

Obsolete AddAsync and change HiLo to generate IDs at SaveChanges time #30957

Open
roji opened this issue May 23, 2023 · 4 comments
Open

Obsolete AddAsync and change HiLo to generate IDs at SaveChanges time #30957

roji opened this issue May 23, 2023 · 4 comments

Comments

@roji
Copy link
Member

roji commented May 23, 2023

We currently have DbSet.AddAsync alongside Add in order to support the HiLo value generator, which must occasionally contact the database to fetch more IDs. This has the following drawbacks:

  • Users are frequently confused about when to use AddAsync vs. Add. IDEs suggest converting Add to AddAsync, but that really is necessary only in the very small minority of cases, when HiLo is used.
  • There are various places in our code where the value generator needs to be invoked, but not all those places have an async context; as a result, we sometimes call the sync Next on the value generator instead of NextAsync, and the user has no way of avoiding sync I/O (#30936). Allowing async everywhere would require adding async overloads to many user APIs (e.g. there's no AttachAsync, DetectChangesAsync).

Instead of this, we can generate a temporary value during Add, and generate the permanent value during SaveChanges, allowing us to obsolete AddAsync.

The main drawback is that entity types will no longer have a (permanent) value after Add, but only after SaveChanges. Since this is already the state of affairs for the default database-based IDs (IDENTITY), and users haven't complained about it, we think that should be OK. Since the usage of HiLo is also generally low, we don't think the breaking change impact is significant.

Note #30753 which is related, proposing switching from client-side GUID generation to server-side. The main objection there is the moving of the (permanent) value generation from Add to SaveChanges time as well.

@stevendarby
Copy link
Contributor

stevendarby commented May 23, 2023

What would this mean for custom I/O value generators?

Currently we implement Next (because we have to) with sync I/O and NextAsync/NextValueAsync with async I/O, and we try to always use AddAsync so we use the async path. Would these changes lead to Next being called on Add and NextAsync being called on SaveChangesAsync, i.e. two I/O calls? Would you have to issue strong guidance that developers update their Next to only produce temporary values, like you plan to do with your Hi-Lo generator?

@roji
Copy link
Member Author

roji commented May 23, 2023

@stevendarby we haven't yet worked out the details - but there will likely be a new type of value generator that can generate both temporary values (during Add, sync only) and permanent ones (later during SaveChanges, sync and async). The current style of value generator would likely continue working at least for a while.

Out of curiosity, can you share some details on your custom I/O value generaotr? We've seen very little of these from users.

@stevendarby
Copy link
Contributor

We consider the DB design and use of value generator to support it as tech debt we essentially want to remove, so to be clear, I don't present this as good case for an I/O generator - I was just hoping there'd be a way to keep it working if you get to this issue before we get to ours 😄

But fwiw, we have 'partitioned' sequences on some tables - e.g. imagine a sequence on Posts that is incremented per Blog. We get the next value by querying Posts for the current max value, filtered by Blog, and adding one 😐

@roji
Copy link
Member Author

roji commented May 23, 2023

I was just hoping there'd be a way to keep it working if you get to this issue before we get to ours

I wouldn't necessarily work too much about that 😉

We get the next value by querying Posts for the current max value, filtered by Blog, and adding one

That sounds like it could have some concurrency issues... But thanks for the detail!

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

No branches or pull requests

3 participants