Skip to content

Add CancellationToken - #51

Merged
nblumhardt merged 5 commits into
datalust:devfrom
SimonCropp:dev
Apr 21, 2018
Merged

Add CancellationToken#51
nblumhardt merged 5 commits into
datalust:devfrom
SimonCropp:dev

Conversation

@SimonCropp

Copy link
Copy Markdown
Contributor

No description provided.

@nblumhardt

Copy link
Copy Markdown
Member

This looks great. I'll need to comb over it with fresh eyes, but with a skim it seems to hit the mark nicely. Many thanks!

@SimonCropp

Copy link
Copy Markdown
Contributor Author

Note this a breaking change at the runtime level

@nblumhardt

Copy link
Copy Markdown
Member

👍 thanks for the note. Yes, the code on dev is currently up a major version from master, and has a few binary-breaking changes already.

@nblumhardt nblumhardt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good - just one query. Cheers!

Comment thread src/Seq.Api/SeqConnection.cs Outdated
_root = new Lazy<Task<RootEntity>>(() => _client.GetRootAsync());

_root = new Lazy<Task<RootEntity>>(() => _client.GetRootAsync(token));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this one, since the token could hypothetically need to be kept around until some unspecified later time.

Perhaps, since it's not an operation that's likely to take long anyway, we just ignore cancellation for now, here.

The other option is to stop using Lazy and instead use a lock, field, and helper method so that the call to GetRootAsync() uses the cancellation token of whatever method is calling it. Perhaps a nice addition down the track, unless you're particularly keen :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, since it's not an operation that's likely to take long anyway, we just ignore cancellation for now, here.

i think that is reasonable. want me to make the change?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, thanks 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@nblumhardt

Copy link
Copy Markdown
Member

Thanks!

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.

2 participants