Tearth/Oddity

Asynchronous support

Opened this issue · 2 comments

Deleted user commented

Seeing the code call await ExecuteAsync().Result scared me 😲 (if you didn't know, calling .Result before the method is finished executing (because of the await) can cause asynchronous issues). A useful link: https://montemagno.com/c-sharp-developers-stop-calling-dot-result/

I can't help but ask how you feel about that, because it breaks the asynchronous pattern.
I'm happy to help with reworking the asynchronous infrastructure, especially because I would like to see fully asynchronous methods available to the user.

Solutions:

  • The idea of the Execute method on the BuilderBase class is definitely a good idea but it's sort of negating the point of the Task-based asynchronous pattern. It would be nice to see methods like Execute and ExecuteAsync, with the former using <AsyncMethodName>().GetAwaiter().GetResult() (the "correct" way to synchronously wait for async methods to complete) and the later using an async requests library
  • Similar to other APIs and libraries (for example, Discord.NET), pushing the Execute level up to the method is a good idea. For example, we could use GetNextAsync and GetNext directly on the builder class.
Tearth commented

Your observations are definitely true, my experience with doing asynchronous methods is a bit little. I will learn more about it and fix in the future version.

Thanks for the advices!

Tearth commented

I've made some changes in 1.0.2 version. Now synchronized version of method uses ExecuteBuilder().GetAwaiter().GetResult(), and every builder implements only one version of method (async) instead of two (sync and async) which is called by BuilderBase. Let me know if it's a move in the right direction.