Skip to content

Use ValueTasks instead of Tasks #81

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

Merged
merged 4 commits into from
Nov 30, 2024

Conversation

HarryCordewener
Copy link
Contributor

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

This is a Binary-Breaking change, and will require the Extensions to get a similar update and refer explicitly to this version and above.

@coronabytes coronabytes merged commit 1e70b2f into coronabytes:master Nov 30, 2024
1 check passed
@HarryCordewener HarryCordewener deleted the Use-ValueTasks branch December 2, 2024 15:46
@Lozko1991
Copy link

Does it make sense to have ALL methods return ValueTask in our API?
I have checked this code and cant understand why we have ValueTask there

public async ValueTask<T> SendAsync<T>(HttpMethod m, string url, object body = null,
    string transaction = null, bool throwOnError = true, bool auth = true,
    IDictionary<string, string> headers = null,
    CancellationToken cancellationToken = default)
{
    await Authenticate(auth, cancellationToken).ConfigureAwait(false); // this method has sync way, its ok

    var msg = new HttpRequestMessage(m, _configuration.Server + url);
    ApplyHeaders(transaction, auth, msg, headers);

    if (body != null)
    {
        var json = _configuration.Serializer.Serialize(body);
        msg.Content = new StringContent(json, Encoding.UTF8,
            "application/json");
    }
    else
    {
        msg.Headers.Add(HttpRequestHeader.ContentLength.ToString(), "0");
    }

    // but here we always have the really asyn call
    var res = await _httpClient.SendAsync(msg, cancellationToken).ConfigureAwait(false); 

    if (!res.IsSuccessStatusCode)
        if (throwOnError)
        {
            var errorContent = await res.Content.ReadAsStringAsync().ConfigureAwait(false);
            var error = _configuration.Serializer.Deserialize<ErrorResponse>(errorContent);
            throw new ArangoException(errorContent, error.ErrorMessage,
                (HttpStatusCode) error.Code, (ArangoErrorCode) error.ErrorNum);
        }
        else
        {
            return default;
        }

    var content = await res.Content.ReadAsStringAsync().ConfigureAwait(false);

    if (res.Headers.Contains("X-Arango-Error-Codes"))
    {
        var errors = _configuration.Serializer.Deserialize<IEnumerable<ErrorResponse>>(content)
            .Select(error => new ArangoError(error.ErrorMessage, (ArangoErrorCode) error.ErrorNum));
        throw new ArangoException(content, errors);
    }

    if (content == "{}" || string.IsNullOrWhiteSpace(content))
        return default;

    return _configuration.Serializer.Deserialize<T>(content);
}

await Authenticate(auth, cancellationToken).ConfigureAwait(false); // this method has sync way, its ok

// but here we always have the really asyn call
var res = await _httpClient.SendAsync(msg, cancellationToken).ConfigureAwait(false);

So, would it be better to leave high-level API with Tasks, and use ValueTasks only in places, where exist ways to run synchronously?

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 this pull request may close these issues.

3 participants