Skip to content

Change blocking methods to use await Task rather than Task.Result #118

Closed
jen20 opened this Issue Apr 26, 2014 · 10 comments

3 participants

@jen20
Event Store member
jen20 commented Apr 26, 2014

As per Andrew Browne's post on the mailing list, and @gregoryyoung's further investigation:

public async Task Lockup(IEventStoreConnection connection)
{
    // await the first read
    Console.WriteLine("reading non-existent stream");
    await connection.ReadStreamEventsForwardAsync("NonExistantStream1", 0, 100, false);

    string randomStreamIdToWriteTo = "LockTestStream-" + Guid.NewGuid().ToString();
    // await a random write
    Console.WriteLine("reading " + randomStreamIdToWriteTo + " stream");
    await connection.AppendToStreamAsync(randomStreamIdToWriteTo, -1, _eventData);

    // this read would succeed if awaited but not if called synchronously
    // instead the code will hang
    Console.WriteLine("reading non-existent stream");
    await connection.ReadStreamEventsForwardAsync("NonExistantStream1", 0, 100, false);
    Console.WriteLine("done reading non-existent stream");
}

Does not hang

public async Task Lockup(IEventStoreConnection connection)
{
    // await the first read
    Console.WriteLine("reading non-existent stream");
    await connection.ReadStreamEventsForwardAsync("NonExistantStream1", 0, 100, false);

    string randomStreamIdToWriteTo = "LockTestStream-" + Guid.NewGuid().ToString();
    // await a random write
    Console.WriteLine("reading " + randomStreamIdToWriteTo + " stream");
    await connection.AppendToStreamAsync(randomStreamIdToWriteTo, -1, _eventData);

    // this read would succeed if awaited but not if called synchronously
    // instead the code will hang
    Console.WriteLine("reading non-existent stream");
    var x = connection.ReadStreamEventsForwardAsync("NonExistantStream1", 0, 100, false).Result;
    Console.WriteLine("done reading non-existent stream");
}

Does hang.

We should change to using await internally in the blocking versions as there are apparently issues with using .Result as described here: http://msdn.microsoft.com/en-us/magazine/jj991977.aspx

@gregoryyoung
Event Store member
@jen20
Event Store member
jen20 commented Apr 26, 2014

I'd tend to agree. Does anyone else have an opinion on this?

@bartelink

Def remove - chasing bugs in Sync-over-async bodges is a fools errand.

Stephen Toub has a set of posts, the exec summary of which is 'don't do it'. They are down but here's a cached example

... As long as you don't need to publish examples in C# <5 (or F# pre 2008 :P) - things get v ugly v fast if you aint going to be able to use async/await).

@jen20
Event Store member
jen20 commented Apr 26, 2014

If we're going to do this, we'd better try to get it into 3.0.0 as it will likely break a lot of people. Probably for the best overall though.

@jen20
Event Store member
jen20 commented Apr 26, 2014

The only method that needs some thought is SubscribeToAllFrom which isn't inherently asynchronous anyway.

@gregoryyoung
Event Store member
@jen20
Event Store member
jen20 commented May 1, 2014

This is now completed.

@jen20 jen20 closed this May 1, 2014
@gregoryyoung
Event Store member
@jen20
Event Store member
jen20 commented May 1, 2014

Ah, yes they do need some attention. Also we might want to look at the catch-up subscriptions as well, though I don't think they're a huge concern as they are right now.

@jen20 jen20 reopened this May 1, 2014
@jen20 jen20 added this to the v3.0.0 milestone Aug 31, 2014
@jen20 jen20 added a commit that referenced this issue Sep 7, 2014
@jen20 jen20 Remove synchronous wrappers on transaction
Remove the synchronous wrapper methods for writing and commiting a
transaction in the client API. Starts to address #118. Fixes tests which
were relying on the synchronous variants.
881a998
@jen20
Event Store member
jen20 commented Sep 7, 2014

Fixed by #212. Catch-up subscriptions probably don't need this.

@jen20 jen20 closed this Sep 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.