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

Problematic implementation of "async" methods #613

Closed
guifuega opened this issue Jun 29, 2022 · 4 comments
Closed

Problematic implementation of "async" methods #613

guifuega opened this issue Jun 29, 2022 · 4 comments
Assignees
Labels

Comments

@guifuega
Copy link

guifuega commented Jun 29, 2022

Hello there:

I came into a problem with the implementation of some of this library's so called "async" methods, declared on DynamicEnumerableAsyncExtensions.cs , like "ToDynamicListAsync", so I decided to create an account just to talk about it.

The thing is I was really expecting these methods to be truly asynchronous. Of course, before using them I did not know the details of their implementation (and I think no one should care about them as we assume they are susceptible to change over time).

Usually I only worry about documentation and method names unless I suspect the method's declaration/documentation is lying (as I think it is the case).

I find it very dangerous for third-party libraries to create new threads from the pool, specially when they just do it without any real need, just to achieve a fake asynchronous implementation, which does not add any real feature to the library.

What is the point of providing methods like these? When a library offers asynchronous methods what I expect from them is a truly asynchronous implementation with real benefits.

By creating new threads without alerting the users of the library, it can lead to unwanted results and it is not the best use of system resources, which can probably have a negative impact on performance. The threadpool is an app-global resource, so you should leave the callers of your library to decide how and when they want to use it.

Using these methods in certain scenarios gave me some problems, cause their implementations are not truly asynchronous and I was not expecting them to create new threads under the hood. Threads I do not have control over.

As I said before, I do not think the library benefits from these methods and they are very likely to cause unwanted and unnecessary problems, so I would encourage you to remove them, as anybody can just implement this asynchronous versions on their own, with a more appropriate and manageable usage of resources depending on their needs.

PS: What was the motivation for creating these methods and why was this implementation chosen? Investigating a little bit, I found this old request: Async support for ToDynamicList() #96 , but it made no sense. As it was stated on that post, the asynchronous implementation would not provide any real benefit, and even so, it was included in the library, introducing the aforementioned potential problems it may lead to and no real benefit for developers.

Thank you in advance for reading and maybe considering this humble request!

Regards.

@StefH
Copy link
Collaborator

StefH commented Aug 3, 2022

Hello @guifuega, thank you for your observation. I'll take a look at the implementation and report back here.

@StefH StefH self-assigned this Aug 3, 2022
@StefH StefH added the question label Aug 3, 2022
@StefH
Copy link
Collaborator

StefH commented Aug 4, 2022

@guifuega

I did update the code, please check the PR:
#620

@StefH
Copy link
Collaborator

StefH commented Aug 14, 2022

Hello @guifuega, did you have to time to verify the PR?

@StefH
Copy link
Collaborator

StefH commented Aug 17, 2022

PR is merged

@StefH StefH closed this as completed Aug 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants