-
Notifications
You must be signed in to change notification settings - Fork 933
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
WIP - Add support for IAsyncEnumerable #2274
base: master
Are you sure you want to change the base?
Conversation
src/NHibernate/Impl/SessionImpl.cs
Outdated
|
||
using (SuspendAutoFlush()) //stops flush being called multiple times if this method is recursively called | ||
{ | ||
return plan.PerformAsyncIterate<T>(queryParameters, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how this will behave....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you talking about? Is it about using (SuspendAutoFlush())
? Yeah this code seems suspicious - if this suspend is really required ( as this code looks broken to me even for sync case - it's surely not scoped for items iterations) it needs to be done inside iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'm talking about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As now PerformAsyncIterate
and PerformIterate
do not execute any query, the auto flush logic was moved into the initialization of the enumerator, so that the auto flush logic will be triggered before the query is executed.
I checked how hibernate does that and they are also suspending only for the first query that is executed. I didn't found any scenario that would need to perform this logic inside the iteration.
Looks good so far. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
src/NHibernate/Impl/SessionImpl.cs
Outdated
|
||
using (SuspendAutoFlush()) //stops flush being called multiple times if this method is recursively called | ||
{ | ||
return plan.PerformAsyncIterate<T>(queryParameters, this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are you talking about? Is it about using (SuspendAutoFlush())
? Yeah this code seems suspicious - if this suspend is really required ( as this code looks broken to me even for sync case - it's surely not scoped for items iterations) it needs to be done inside iterator.
} | ||
|
||
bool sessionDefaultReadOnlyOrig = _session.DefaultReadOnly; | ||
_session.DefaultReadOnly = _readOnly; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to do it for each processed item? Maybe it should be done once for iterations (so this logic should be moved to GetAsyncEnumerables
or even to JoinedAsyncEnumerable
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really make sense to do it for each processed item?
The user may change the DefaultReadOnly
value while iterating, which may cause a readonly query to load entites as writable, if DefaultReadOnly
is not set for each item.
So, with the latest iteration of changes this has become a close resemblance of "Future" feature, right? |
The last iteration of changes applied the same logic in terms of when the query is executed to the current |
Rebased and re-generated with AsyncGenerator |
Only now I realized that
will trigger one query that will retrieve all ids and then on each |
Rebased. |
That sucks :( It's not very useful for entities but I guess it's still can be useful for scalar values...
Yeah. And I would just replace Enumerable implementation with scroll. Current enumerable entity handling is not intuitive at all. |
One issue that I didn't checked, is how |
The main purpose of the Future feature is not deferred execution but batching together queries. (Future also features a very loose coupling between code parts defining the queries, contrary to the recent query batcher which requires referring to the same batcher reference for adding queries into it, but as such also helps knowing exactly what is batched together.) This Anyway, it appears this "enumerable" feature is not very attractive with its current performances issues. Is it the reason for this PR to be stalled? Or should its conflicts be resolved for allowing merging it? |
Yes, the |
Is there an issue to track the porting of the |
Any progress here with the scroll feature? |
Limitations: The added Linq extension method
AsAsyncEnumerable
do not supportPostExecuteTransformer
as the transformer operates withIEnumerable<>
.Fixes: #2267
Added WIP as the async generator has some issues generating obsolete overloads and tests with
IAsyncEnumerable
.