Async support within Glimpse #486

Merged
merged 2 commits into from Oct 30, 2013

Conversation

Projects
None yet
4 participants
@dahlbyk
Contributor

dahlbyk commented Jul 26, 2013

Props to @loudej for suggestion of CallContext.Logical_etData()

/cc @xpaulbettsx is there a more correct way to wrap and time the async operation?

@paulcbetts

This comment has been minimized.

Show comment
Hide comment
@paulcbetts

paulcbetts Jul 26, 2013

Here, you're starting new task threads, then making them block on an async method. Instead, you should keep the whole thing async:

public override async Task<object> ExecuteScalarAsync(CancellationToken cancellationToken)
{
    EnsureConfiguration();

    object result;
    var commandId = Guid.NewGuid();

    var timer = this.LogCommandSeed();
    this.LogCommandStart(commandId, timer);
    try
    {
        result = await InnerCommand.ExecuteScalarAsync();
    }
    catch (Exception exception)
    {
        this.LogCommandError(commandId, timer, exception, "ExecuteScalarAsync");
        throw;
    }
    this.LogCommandEnd(commandId, timer, null, "ExecuteScalarAsync");

    return result;
}

Here, you're starting new task threads, then making them block on an async method. Instead, you should keep the whole thing async:

public override async Task<object> ExecuteScalarAsync(CancellationToken cancellationToken)
{
    EnsureConfiguration();

    object result;
    var commandId = Guid.NewGuid();

    var timer = this.LogCommandSeed();
    this.LogCommandStart(commandId, timer);
    try
    {
        result = await InnerCommand.ExecuteScalarAsync();
    }
    catch (Exception exception)
    {
        this.LogCommandError(commandId, timer, exception, "ExecuteScalarAsync");
        throw;
    }
    this.LogCommandEnd(commandId, timer, null, "ExecuteScalarAsync");

    return result;
}
@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Jul 26, 2013

Contributor

Rebased back against an older version because newer master builds are done broked.

@xpaulbettsx I swear I'd seen a warning about marking an override method as async, but I must have had issues elsewhere - how does 54650c0?w=0 look?

Contributor

dahlbyk commented Jul 26, 2013

Rebased back against an older version because newer master builds are done broked.

@xpaulbettsx I swear I'd seen a warning about marking an override method as async, but I must have had issues elsewhere - how does 54650c0?w=0 look?

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 18, 2013

Contributor

Do you need anything else from me on this? Should I rebase away my silliness?

Contributor

dahlbyk commented Sep 18, 2013

Do you need anything else from me on this? Should I rebase away my silliness?

@nikmd23

This comment has been minimized.

Show comment
Hide comment
@nikmd23

nikmd23 Sep 20, 2013

Member

I think we are good to move forward on this @dahlbyk.

A rebase would be dandy - thanks!

Member

nikmd23 commented Sep 20, 2013

I think we are good to move forward on this @dahlbyk.

A rebase would be dandy - thanks!

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Sep 21, 2013

Contributor

Rebased

Contributor

dahlbyk commented Sep 21, 2013

Rebased

@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 29, 2013

Member

Ok. I've tested this and it looks like it works :D

One thing I noticed before I pull it in is that GlimpseDbCommand.cs has a lot of white space changes. I know this might be picky, but any chance you could undo those before I pull it in? Sorry :(

Member

avanderhoorn commented Oct 29, 2013

Ok. I've tested this and it looks like it works :D

One thing I noticed before I pull it in is that GlimpseDbCommand.cs has a lot of white space changes. I know this might be picky, but any chance you could undo those before I pull it in? Sorry :(

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Oct 29, 2013

Contributor

The whitespace changes are alone in 1cfe18c - I'll discard it.

Contributor

dahlbyk commented Oct 29, 2013

The whitespace changes are alone in 1cfe18c - I'll discard it.

dahlbyk added some commits Jul 23, 2013

GlimpseDbCommand Async support
Tips of the hat to:
* @loudej for suggesting CallContext.Logical_etData()
* @paulcbetts for async advisement
@avanderhoorn

This comment has been minimized.

Show comment
Hide comment
@avanderhoorn

avanderhoorn Oct 29, 2013

Member

GREAT! Thanks a lot. The fix seems to be working well.

On Tue, Oct 29, 2013 at 5:24 PM, Keith Dahlby notifications@github.comwrote:

The whitespace changes are alone in 1cfe18chttps://github.com/Glimpse/Glimpse/commit/1cfe18c- I'll discard it.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/486#issuecomment-27344892
.

Member

avanderhoorn commented Oct 29, 2013

GREAT! Thanks a lot. The fix seems to be working well.

On Tue, Oct 29, 2013 at 5:24 PM, Keith Dahlby notifications@github.comwrote:

The whitespace changes are alone in 1cfe18chttps://github.com/Glimpse/Glimpse/commit/1cfe18c- I'll discard it.


Reply to this email directly or view it on GitHubhttps://github.com/Glimpse/Glimpse/pull/486#issuecomment-27344892
.

@dahlbyk

This comment has been minimized.

Show comment
Hide comment
@dahlbyk

dahlbyk Oct 29, 2013

Contributor

🎱

Contributor

dahlbyk commented Oct 29, 2013

🎱

avanderhoorn added a commit that referenced this pull request Oct 30, 2013

Merge pull request #486 from dahlbyk/AdoAsync
Async support within Glimpse

@avanderhoorn avanderhoorn merged commit 05aa056 into Glimpse:master Oct 30, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment