BaseXdb / basex

BaseX Main Repository.
http://basex.org
BSD 3-Clause "New" or "Revised" License
661 stars 268 forks source link

Add async functionality to C# client #2169

Closed binarycow closed 1 year ago

binarycow commented 1 year ago

This PR adds support to the C# client for async/await.

ChristianGruen commented 1 year ago

Thanks for the pull request. Two suggestions:

binarycow commented 1 year ago

As asynchronous streams were only introduced with C# 8.0

I'm not using IAsyncEnumerable (the feature that was introduced in C# 8).

I did update the language version in the MR, but that doesn't require users to update too - if we publish a nuget library. If they're just copy/pasting the source, they'll need to adjust, or upgrade their language version

this client could be offered as an alternative solution and moved to a new src/main/c#-async/ directory.

I was hoping we could get this in a nuget package. No reason we can't put it all in one.

  • I think the code could then be deduplicated

Yes, it can. But I didn't want to modify the existing code too much, and I made the async mirror the synchronous

the original functions could be replaced with the asynchronous variants (unless you believe there are use cases in which both variants are required?).

Yes, you need both. It's a bad idea in C# to try to mix async and synchronous code.

So, if we remove the synchronous code, it's a deadlock waiting to happen for anyone who isn't using async.

ChristianGruen commented 1 year ago

I'm not using IAsyncEnumerable (the feature that was introduced in C# 8).

Thanks, I see. Sounds ok then.

Yes, you need both. It's a bad idea in C# to try to mix async and synchronous code. So, if we remove the synchronous code, it's a deadlock waiting to happen for anyone who isn't using async.

To understand this better: How would you expect synchronous requests to happen if we limit the client code to asynchronous functions?

Currently, most existing client bindings in our repository follow the conventions of our documentation on language clients. Before we add the client code to the repository, it would be perfect if you could add a readme file that comments on the specifics of the updated C# client and how it differs from the standard features.

binarycow commented 1 year ago

To understand this better: How would you expect synchronous requests to happen if we limit the client code to asynchronous functions?

If the synchronous code were removed, then a synchronous client would need to use the Result property on the Task objects. This is a bad idea. Very prone to deadlocks. You can read more about it here.

Removing the synchronous code is not a good move - I strongly recommend we do not do that. Both synchronous and asynchronous options should be available.

Before we add the client code to the repository, it would be perfect if you could add a readme file that comments on the specifics of the updated C# client and how it differs from the standard features.

It is the exact same features. Just now there's an async option. For every synchronous method, there is now an asynchronous method. 1 to 1 correspondence.

ChristianGruen commented 1 year ago

I see. Thanks.

A little readme would still be helpful, though. In addition, please add your name as a contributor to the headers of the files, so people know who to contact if questions arise.

ChristianGruen commented 1 year ago

@binarycow Would you be interested in having your name as a contributor in the source files, and writing a little readme file?