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

Add support for IAsyncEnumerable #2267

Open
cremor opened this issue Nov 8, 2019 · 13 comments · May be fixed by #2274
Open

Add support for IAsyncEnumerable #2267

cremor opened this issue Nov 8, 2019 · 13 comments · May be fixed by #2274

Comments

@cremor
Copy link
Contributor

cremor commented Nov 8, 2019

C# 8.0 and .NET Standard 2.1 brought us asynchronous streams (horrible name, I'll call them "async enumerables" from now on). It would be nice if NHibernate could add new methods that return an IAsyncEnumerable<T>. This would allow us to iterate over the results with await foreach.

I know this won't be a simple task and will require a few preparation steps, like adding .NET Standard 2.1 (and maybe .NET Core 3.0) targets to the build process. (Maybe this would even be the first time that NHibernate has a different API across target frameworks? edit: .NET Standard 2.0 and .NET Framework support IAsyncEnumerable with Microsoft.Bcl.AsyncInterfaces.) But I thought that this issue could at least be a tracking issue for the overall progress.

@fredericDelaporte
Copy link
Member

I think it would have to be done for NHibernate 6.0, which may simply drop .Net Framework v4. (So the plan would then be to wait for .Net 5 and target this one for NHibernate 6.)

Otherwise we may end-up with a lot of conditional compilation directives in code, because there is also Span<T> and other features which will be tempting to use with 2.1.

@cremor
Copy link
Contributor Author

cremor commented Nov 13, 2019

Waiting for NHibernate 6.0 to add IAsyncEnumerable methods sounds reasonable.

OT:
I'm not so sure about dropping both the .NET Framework and .NET Standard 2.0 targets, that would mean loosing many projects that use NHibernate today. In my opinion, at least .NET Standard 2.0 should stay a target framework, so that projects targeting a recent version of .NET Framework can still use NHibernate.

@fredericDelaporte
Copy link
Member

Maybe there is a misunderstanding here: I do not see this as dropping .Net Framework, since we would target .Net 5, which is meant to be the successor of .Net Framework v4.8 (and of .Net Core 3 altogether).
So that would be a change a bit like dropping support for .Net Framework v2/v3.5 in favor of v4. At some point we will have to do it anyway.

@hazzik
Copy link
Member

hazzik commented Nov 14, 2019

We can start using C# 8 now and add support of the interface in the current version. The interfaces are available in the compatibility package.

To be able to do we need to deprecate ToEnumerableAsync (which is broken-ish anyway: it mixes async and sync access to the data provider which could lead to blocking) and make a new method ToAsyncEnumerable which will return IAsyncEnumerable interface.

@hazzik
Copy link
Member

hazzik commented Nov 14, 2019

The only problem I see now: we need a support from @maca88’s AsyncGenerator so we can start using the new C#8 syntactic sugar

@cremor
Copy link
Contributor Author

cremor commented Nov 15, 2019

@hazzik Good point, I forgot about the compatability package (Microsoft.Bcl.AsyncInterfaces). Thanks for mentioning it!
With the help of this package IAsyncEnumerable methods could indeed be supported for .NET Standard 2.0 and .NET Framework down to 4.6.1.

@fredericDelaporte Dropping .NET Framework 2.0/3.5 was not a huge deal because updating a solution from 2.0/3.5 to 4.0 was as easy as changing the target framework most of the time. In contrast, switching from .NET Framework to .NET 5 will be at least as much work as it is now to switch to .NET Standard/.NET Core.

I know that .NET Framework support has to be dropped sometime. But I wouldn't do it in the foreseeable future.

@maca88 maca88 linked a pull request Nov 16, 2019 that will close this issue
@maca88
Copy link
Contributor

maca88 commented Nov 16, 2019

we need a support from @maca88’s AsyncGenerator so we can start using the new C#8 syntactic sugar

By adding Microsoft.Bcl.AsyncInterfaces we do not gain the ability to use C#8 syntactic sugars like await foreach, so the only thing that the generator could do is to modify the return type. By modifying the return type, the generator would need to transform the creation of the IEnumerable that is returned and all its usages, which is not that easy as there are many possible ways of creating and using an IEnumerable. Here is my attempt to add IAsyncEnumerable without modifying the generator.

@hazzik
Copy link
Member

hazzik commented Nov 16, 2019

By adding Microsoft.Bcl.AsyncInterfaces we do not gain the ability to use C#8 syntactic sugars like await foreach

Obviously not. What I mean is that we need AsyncGenerator support to start using C#8 regardless of async enumerables feature.

@maca88
Copy link
Contributor

maca88 commented Nov 16, 2019

What I mean is that we need AsyncGenerator support to start using C#8 regardless of async enumerables feature.

I agreee, my current plan is to start working on it after 5.3 release.

@cremor
Copy link
Contributor Author

cremor commented Mar 23, 2021

I understand that this might still be far away and not finally decided, but is this feature still planned for NHibernate 6.0? If yes, could someone please assign it to the "next major" milestone?

@maca88
Copy link
Contributor

maca88 commented Mar 24, 2021

is this feature still planned for NHibernate 6.0?

It all depends on scroll feature from Hibernate that has to be ported for #2274 to work properly. I will try to port the scroll feature in the following weeks so that this could be included in the next minor version instead.

@aokellermann
Copy link

what is the status of this feature?

1 similar comment
@kalimuthuswamy
Copy link

what is the status of this feature?

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 a pull request may close this issue.

6 participants