Skip to content

Async operations #228

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

Merged
merged 14 commits into from
May 13, 2021
Merged

Async operations #228

merged 14 commits into from
May 13, 2021

Conversation

vaind
Copy link
Contributor

@vaind vaind commented Apr 27, 2021

closes #49
closes #131

Copy link
Member

@greenrobot-team greenrobot-team left a comment

Choose a reason for hiding this comment

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

Did not spot any obvious issues. Found some docs stuff, potential test improvement and unrelated changes.

/// ID immediately, even though the actual database put operation may fail.
Future<int> putAsync(T object, {PutMode mode = PutMode.put}) async {
if (_hasRelations) {
throw UnsupportedError('putAsync() is currently not supported on entity '
Copy link
Member

Choose a reason for hiding this comment

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

This seems too pessimistic. Would checking the object itself for relation data be too involved here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it wouldn't be too hard to do but it would be much less predictable. I'd like to go with this version and possibly ease up the requirement if there are complaints. BTW objectbox-go does the same

Copy link
Member

@greenrobot greenrobot May 7, 2021

Choose a reason for hiding this comment

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

I think the rule is simple enough to be predictable. I'm OK doing this right after 1.0 but let's not wait for complaints. For Dart, async ops are much more essential than for our other platforms, which have threads and stuff.

Copy link
Contributor Author

@vaind vaind May 10, 2021

Choose a reason for hiding this comment

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

I think the rule is simple enough to be predictable.

Yeah, but there being a rule that only triggers sometimes (based on the actual object you're inserting) means developers will only get the error occasionally, not on any put on that entity, thus may miss the cases where this happens, leading to errors in the app. If they're (un)lucky enough they will notice it later than necessary, because, you know, who reads docs...

Copy link
Member

Choose a reason for hiding this comment

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

I'm perfectly aware and OK with that. Usually dev's either put object with or without relations. Having both options; well, then it was missing a test case in dev's code.

Copy link
Contributor Author

@vaind vaind May 10, 2021

Choose a reason for hiding this comment

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

I'll prep the change, see how that goes

if (id == 0) {
// Note: if the newId future completes with an error, ID isn't set and
// this call throws. Consider using `newId.then()` to avoid the throw?
_entity.setId(object, await newId);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we wait only for "insert" (id == 0) here. Shouldn't we always wait; otherwise user should use the optimistic "fire&forget" variant.

Also, I didn't completely get the comment above this line, probably related, might be we should discuss?

Copy link
Contributor Author

@vaind vaind May 10, 2021

Choose a reason for hiding this comment

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

I've changed the comment, was not really true.

Waiting here doesn't actually change user's program execution - what you're suggesting wouldn't do anything. Consider the following code (you can run it on https://dartpad.dev):

import 'dart:async';

Future<int> fn() async {
  print('hey');
  await Future<void>.delayed(const Duration(milliseconds: 10));
  print('ho');
  return Future.value(10);
}

void main() async {
  final f1 = fn();
  final f2 = fn();
  await f1;
  await f2;
  print('-----');
  await fn();
  await fn();
}

prints

hey
hey
ho
ho
-----
hey
ho
hey
ho

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we are writing about the same thing. For me: "always waiting" as in "see if we get an error"; if we are not interested in error status, this is the wrong function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're talking about the same thing but I think you're missing a piece of understanding how await works (that's why the code example). I'm saying that decision comes down to the user - whether/when they await the returned Future. We can't do anything about that. Adding an await in putAsync() won't do what you're trying to achieve - see if we get an error. Same for the rest: "if we are not interested in error status, this is the wrong function" - I agree but we can't do anything about it - it's up to the caller (developer) to await

Copy link
Member

Choose a reason for hiding this comment

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

Ofc, I missed that we return the same newId we're waiting for.

What's a little odd and providing to my confusion is that we are artificially holding back the new ID. But I guess we cannot return a pair, e.g. <int, Future> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But I guess we cannot return a pair, e.g. <int, Future> ?

not without a confusing custom type for that...

Copy link
Member

@greenrobot greenrobot May 10, 2021

Choose a reason for hiding this comment

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

It's a pity, as having the ID could pave the way to define async relations, as they depend on it...

PS.: could we extend Future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not what prevents us doing relations, it's rather that if you used that ID of an uncompleted future in a related entity, then if the original future failed (e.g. unique violation), the latter would have an invalid relation but would still complete the future with a success. Basically same as with the putQueued where you don't get any info whether it failed or not.

Copy link
Contributor Author

@vaind vaind May 10, 2021

Choose a reason for hiding this comment

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

As is, it's actually pretty useful, just await when assigning the ID to the related entity

Copy link
Member

@greenrobot greenrobot May 11, 2021

Choose a reason for hiding this comment

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

Actually, I think the behavior should be to set the ID in the object immediately to allow referring to it via relations.

PS.: Another pov here is that the missing piece is to have dependent operations (e.g. using unconfirmed "async" IDs) must happen in the same transaction. That's mid-term how-ever. For now, immediate setting in the object with the option to wait for success manually seems reasonable. We shouldn't await each object as there could be a lot of them, and enforcing a transaction on each of them would kill any bulk performance.

@vaind vaind force-pushed the async branch 2 times, most recently from 99f27ca to a230143 Compare May 12, 2021 08:55
@vaind vaind marked this pull request as ready for review May 13, 2021 17:29
@vaind vaind merged commit 2f4f4e7 into main May 13, 2021
@vaind vaind deleted the async branch May 13, 2021 17:30
vaind added a commit that referenced this pull request Aug 2, 2021
vaind added a commit that referenced this pull request Aug 2, 2021
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.

Feature request: async put/get Async/Await/Futures
3 participants