Queries with sync-over-async and synchronization context hang

[lukasz-pyrzyk]

Describe the bug
Continuation of the issue #1043.

It looks like #1043 fixed point-read calls, stored procedure calls, calls for db/container details. However, when client does a query with GetItemLinqQueryable<Entity>().ToFeedIterator(), it still hangs.

To Reproduce
Repro available in the same repository as previously, https://github.com/lukasz-pyrzyk/CosmosDbSyncOverAsync .

Expected behavior
A hang should not occur for query calls

Actual behavior
Second query to the database which is called from synchronization context and blocked with .GetAwaiter().GetResult() hangs.

Environment summary
SDK Version: 3.5.1
OS Version: Windows 10

[ealsur]

Have you tried with allowSynchronousQueryExecution: true ?

https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/SynchronizationContextTests.cs#L76

[j82w]

I think I found the issue. The QueryIterator ReadNextAsync doesn't have the TaskHelper.RunInlineIfNeededAsync helper function.

@ealsur is correct that the better approach here is to just set allowSynchronousQueryExecution: true for the linq query.

[ealsur]

Checking your original code:

var query = dbClient
    .GetItemLinqQueryable<ClientEntity>(requestOptions: new QueryRequestOptions { PartitionKey = partitionKey })
    .Where(x => x.ClientId == clientId)
    .ToFeedIterator();

var client = await query.ExecuteFirstOrDefault(nameof(GetClient)).ConfigureAwait(false);

public static async Task<TQueryItem> ExecuteFirstOrDefault<TQueryItem>(this FeedIterator<TQueryItem> iterator, string operationName)
{
    Trace.WriteLine($"Executing {operationName}");
    var feedResponse = await iterator.ReadNextAsync().ConfigureAwait(false);
    Trace.WriteLine($"{operationName} finished with status code {feedResponse.StatusCode}. Request charge is {feedResponse.RequestCharge}.");

    return feedResponse.FirstOrDefault();
}

This has the issue that you are not draining the query, which could lead to memory leaking. Since you are already declaring the query as Linq, you can just do:

IQueryable<ClientEntity> query = dbClient.GetItemLinqQueryable<ClientEntity>(
    allowSynchronousQueryExecution: true,
    requestOptions: new QueryRequestOptions()
    {
        PartitionKey = partitionKey
    }).Where(x => x.ClientId == clientId);
    
    var client = query.ToList().FirstOrDefault();

We can check the ReadNextAsync but since there is an alternative for sync scenarios, it is not really a blocker.

[lukasz-pyrzyk]

This has the issue that you are not draining the query, which could lead to memory leaking. Since you are already declaring the query as Linq, you can just do:

Can you explain how this can do a memory leaking? Is it a bad pattern to create query, then iterator and call iterator asynchronously?

We can check the ReadNextAsync but since there is an alternative for sync scenarios, it is not really a blocker.

Yes, I can do it. Unfortunately, I think that I'm loosing diagnostic information here, like RU usage.

I think it would be good to have a consistent behavior on different asynchronous operations.

Also, I have a different question. In https://github.com/Azure/azure-cosmos-dotnet-v3/blob/master/Microsoft.Azure.Cosmos/src/TaskHelper.cs#L88 code spins. This task runs outside of the synchronization context, so it fixes the issue. However, wouldn't be more correct to use ConfigureAwait(false) on all async calls in the library? Isn't it a Microsoft recommendation for the libraries that might be used in the project with a synchronization context?

[ealsur]

Can you explain how this can do a memory leaking? Is it a bad pattern to create query, then iterator and call iterator asynchronously?

When you issue a query, you need to loop through the results until HasMoreResults is false, see https://docs.microsoft.com/en-us/azure/cosmos-db/troubleshoot-query-performance#read-all-results-from-continuations (which is for V2 SDK but targets the same scenario). If for whatever reason, there are more results left to read and have been buffered and materialized by the SDK but not consumed, those results will be kept in memory as time passed. As you keep repeating and doing more queries (and not consuming all results), the memory footprint will keep growing.

I agree with the fact that ReadNextAsync should be fixed, I'm just pointing at the fact that it is not urgent when prioritizing versus other work because there is a valid workaround to execute the query (although not ideal, as you pointed out, it does not expose RequestCharge).

Regarding ConfigureAwait, @kirankumarkolli do we have any decision whether we should use ConfigureAwait or not? See https://devblogs.microsoft.com/dotnet/configureawait-faq/