[RFC] Bring back non-Async functions

[kendallb]

One of the things missing in v107 is the sync functions, as those were deprecated to only support async versions. This is yet another pain point in the API surface area for someone migrating from v106 to v107, as you have to re-write all your code to be async, or learn how to correctly call async functions in a sync environment. For lots of legacy code, moving to a full async model is a huge amount of work.

I will implement this in my PR, as I really need this myself to avoid having to re-write all my existing RestSharp code. Internally it will just use an Async helper, but that is better than having to recode it and implement that myself.

[kendallb]

Added to my PR:

#1795

[alexeyzimarev]

@kendallb I think it should be a different PR, #1795 is getting too big. Would you be able to hold the sync overloads, move them to another PR? We also need to decide which way to offload async to sync (benchmark at least).

I'd also like to understand what prevents people from converting their code to async code. I understand the argument about "it can be too much", that's fine. In this case, those overloads should be marked as obsolete and only stay there temporarily, to be removed altogether later on. If there are other reasons, we need to accommodate for those reasons specifically instead.

[kendallb]

@kendallb I think it should be a different PR, #1795 is getting too big. Would you be able to hold the sync overloads, move them to another PR? We also need to decide which way to offload async to sync (benchmark at least).

Sure, I will make another PR for just that. I am using the main PR as my branch for all the things I need to change to get my code working and I am keeping stuff in separate changes so it's easy to patch against dev.

As for benchmarking it, I am not sure it's entirely necessary as many other big projects do it in a similar fashion, including NLog:

https://github.com/NLog/NLog/blob/dev/src/NLog/Common/AsyncHelpers.cs

Nobody uses the task pool approach as I am pretty sure it's much more overhead as the continuation approach does not put any pressure on the thread pool. Finally the point of async functions is that you use them when there is something that can block for reasonable amounts of time (like networking calls) and any minor difference in performance is going to be swamped out by whatever networking calls are being made anyway.

Now at the end of the day NOT doing anything async is better when you do not want it. There is a good discussion with myself and Bradley Grainger who develops the MySqlConnector.net library as I thought using GetAwaiter().GetResult() was safe because I had seen him do it in his code (it's not):

mysql-net/MySqlConnector#960

The reason he is able to use it in his code is that Bradley did is use headers to pass stuff down to the internal I/O pipeline so that the final underlying networking calls either end up calling Sync or Async versions when it hits the wire. Quite smart, as then you can share all the same code, but at the public API level just use GetAwaiter().GetResult() to get the results quickly for non-sync and it never deadlocks because it never actually does anything async in the first place.

Might be worth considering that here for a second iteration?

Here is more on why GetAwaiter().GetResult() does not work:

https://stackoverflow.com/questions/39007006/is-getawaiter-getresult-safe-for-general-use
https://blog.stephencleary.com/2012/07/dont-block-on-async-code.html

I'd also like to understand what prevents people from converting their code to async code. I understand the argument about "it can be too much", that's fine. In this case, those overloads should be marked as obsolete and only stay there temporarily, to be removed altogether later on. If there are other reasons, we need to accommodate for those reasons specifically instead.

No, I don't think you should ever remove them. There is a reason why Microsoft has sync and async versions of all their functions, and why libraries like MySqlConnector have the same. For those who are not doing async, it is a huge amount of work to convert the code, and in many case provides no value until you can get async all the way to the top.

The danger is folks will simply make the mistake I did (and the EasyPost folks did and published it publicly!) is that using GetAwaiter().GetResult() is a viable way to do it.

As for re-writing code to be async, for me we have hundreds of thousands of lines of code that was written non-async over the last 18 years. We do plenty of async code now in places where it has made a material difference to actually use async (ie: we can run stuff in async parallel to get more stuff done in a single thread without needing multiple threads), but we use it only in a few places where it makes sense. Over time we plan to convert the entire code base, but the reason we don't just convert it all over, is it is a huge amount of work for no real value.

Just say you have a function that makes a REST call in a low level class, and you need to make it async. So you convert the call to ExecuteAsync() and now you have to decide, do I convert this entire function now to Async? Or do I use an AsyncHelper() wrapper to make it run sync? If you convert the function itself to async, you might find now you have to convert a few other bits in that function to async, but worse you have to convert all the code that calls into it to async as well. Then for each of those, you need to make the same decision all the way until you pop out the top of the call stack, whether that is an ASP.NET controller, or Windows Forms event handler, Android event handler, Console main function or whatever. At some point you get to the very top and it's async supported at the .NET API level, and you are good. But for any large code base, including ours, the amount of code to convert grows exponentially depending on how deep the functions are where you need to convert to async (and generally REST calls are buried very deep in the stack trace of most code).

I actually started on it myself. I thought hey, why not just try converting it. After about an hour converting one API (CloudFlare) to async, and then tracing back all the code that now also needed to become async, I realized it was a pointless exercise at this moment in time and much easier to simply put non-async functions back into RestSharp. And I am sure many other developers will continue to use the non-async functions also if they are available (and it would have avoided EasyPost making the mistake they did as well).

[kendallb]

New PR:

#1800

[kendallb]

As for the approach used my MySqlConnector, I don't think that is viable at all because there are async callbacks in for stuff like authentication and OnBeforeRequest and OnAfterRequest that are async and that would be user code we cannot control. so the only two viable options are using an AsyncHelper wrapper, or duplicating all the internal functions again to do to both. I think the wrapper approach makes the most sense.

[alexeyzimarev]

Sounds good.

Just a note, from the discussion in mysql:

More specifically, MySqlConnector only calls GetAwaiter().GetResult() on already-completed tasks

It sounds weird to me as if the task is completed, one can just get .Result, it's already there. No need to get an awaiter at all.

[alexeyzimarev]

As for the approach used my MySqlConnector, I don't think that is viable at all because

I agree, as we should keep the internals async and put the wrapper at the Execute level.

[kendallb]

Just a note, from the discussion in mysql:

More specifically, MySqlConnector only calls GetAwaiter().GetResult() on already-completed tasks

It sounds weird to me as if the task is completed, one can just get .Result, it's already there. No need to get an awaiter at all.

Yeah, not clear on that myself. But it seems it's different (something to do with wrapping of exceptions):

https://stackoverflow.com/questions/17284517/is-task-result-the-same-as-getawaiter-getresult

[alexeyzimarev]

That's right, but it would be faster to check if the task has failed when it's already completed. Nevertheless, although it's interesting, it's not really relevant :)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[repo-ranger]

⚠️ This issue has been marked wontfix and will be closed in 3 days

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.