Using async/await in System.Interactive.Async

[aelij]

Hi,

This PR is a precursor to implementing all the operators with async delegates. With async/await this will be very easy to implement. It also simplifies the code a great deal.

For example:

IAsyncEnumerable<T> WhereAsync<T>(this IAsyncEnumerable<T> enumerable, Func<T, Task<bool>> predicate)

My motivation is the release of Service Fabric, which exposes its own IAsyncEnumerable<T> interface. With a simple wrapper (such as the one here) it would be possible to use System.Interactive.Async with it. Since SF reliable collections only expose an async enumerable, this is the only option to use LINQ. The async delegate versions of the operators are also very useful in cross-collection queries (as you can see in the aforementioned Gist).

I realize this is a pretty big (unsolicited ;) change, but hopefully you'll consider it.

Eli.

---

This change is

[clairernovotny]

Hi @aelij, can you please rebase this on to the EF-Fixes branch?

[aelij]

Sure. Does it mean I'll need to open a new PR?

[clairernovotny]

@aelij if you rebase and force push to the same branch, it'll keep it associated with this PR

[clairernovotny]

Does this PR only contain async/await impls or does it also include things like WhereAsync? I'd keep new methods as a separate discussion/PR.

[aelij]

Just async/await for current methods.

[clairernovotny]

I was about to do the same in the EF-Fixes branch, so I'm glad I noticed this :) The biggest thing in that branch is a copy/use of Lookup<T,V> to better implement GroupJoin

[aelij]

@onovotny, I'm trying to run the tests after the rebase with no success. The Test Explorer doesn't discover them, ReSharper won't run project.json test projects at all, and dotnet test produces the following error:

System.Interactive.Async>dotnet test
dotnet-test Error: 0 : System.InvalidOperationException: Can not find runtime target for framework '.NETStandard,Version=v1.0' compatible with one of the target runtimes: 'win10-x64, win81-x64, win8-x64, win7-x64'. Possible causes:
1. The project has not been restored or restore failed - run `dotnet restore`
2. The project does not list one of 'win10-x64, win81-x64, win8-x64, win7-x64' in the 'runtimes' section.
   at Microsoft.DotNet.ProjectModel.BuildWorkspace.GetRuntimeContext(ProjectContext context, IEnumerable`1 runtimeIdentifiers)
   at Microsoft.DotNet.Tools.Test.TestCommand.<>c__DisplayClass2_0.<DoRun>b__0(ProjectContext c)
   at System.Linq.Enumerable.WhereSelectListIterator`2.MoveNext()
   at System.Collections.Generic.List`1..ctor(IEnumerable`1 collection)
   at System.Linq.Enumerable.ToList[TSource](IEnumerable`1 source)
   at Microsoft.DotNet.Tools.Test.TestCommand.DoRun(String[] args)

Any idea how to fix this? Thanks.

[clairernovotny]

For the code currently in master/EF-Fixes, it requires a newer set of CLI tools -- basically ones close to the RTM. You should be able to get the latest .NET Core SDK installer from here:

https://github.com/dotnet/cli

BTW, on/after Monday, you'll want to run nuget locals all -clear to ensure you have the "real" versions of the final packages.

[aelij]

Can this wait until Monday? I'd rather not install nightlies :)

[clairernovotny]

Indeed, thanks!

[aelij]

I've run the tests, and all of them pass except Do5 and Do6. This is because the exception received in delegate of the Do operator is no longer an AggregateException but rather the inner one, as thrown by the async operator.

The question now is whether we take this breaking change and conform to the async way, or keep the original behavior.

WDYT?

[clairernovotny]

@aelij Changing from AggregateException to the inner one is ok and would be expected.

[aelij]

I've force-pushed the branch. Cool, I see you've added CI builds!

Regarding the Do change, if you're planning on writing some release notes, this should be mentioned.

[shiftkey]

Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 102 [r3] (raw file):

to ignore it for now here

So we should leave this as-is or undo this change?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 358 [r3] (raw file):

                        if (await e.MoveNext(cts.Token).ConfigureAwait(false))
                        {
                            if (predicate(e.Current))

💄 return predicate(e.Current); for a bit of concise-ness?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 392 [r3] (raw file):

                        if (await e.MoveNext(cts.Token).ConfigureAwait(false))
                        {
                            if (predicate(e.Current, checked(index++)))

💄 return predicate(e.Current, checked(index++)); for a bit of concise-ness?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 900 [r3] (raw file):

                    {
                        res = await e.MoveNext(ct).ConfigureAwait(false);
                        if (res == true)

💄 do we need the == true bit here?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1120 [r3] (raw file):

                    {
                        exception?.Throw();
                        return false;

I think this is fine to leave here, as the old code using TaskCompletionSource meant that TrySetException wasn't going to throw the exception (but .Throw() here should).

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1232 [r3] (raw file):

                        return result;
                    }
                    catch (OperationCanceledException)

Can we document why this is an important scenario to catch and rethrow explicitly, rather than fall through to the onError?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1299 [r3] (raw file):

                while (await e.MoveNext(cancellationToken).ConfigureAwait(false))
                {
                    action(e.Current, checked(index++));

This is an introduced checked here - is this just to be consistent or did you encounter something unexpected here during the migration?

---

Ix.NET/Source/System.Interactive.Async/EmptyArray.cs, line 1 [r3] (raw file):

// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved. See License.txt in the project root for license information.

As this is a new file, please use the new license header:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information. 

---

Ix.NET/Source/System.Interactive.Async/Grouping.cs, line 1 [r3] (raw file):

using System;

As this is a new file, please use the new license header:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information. 

---

Ix.NET/Source/System.Interactive.Async/Grouping.cs, line 9 [r3] (raw file):

namespace System.Linq.Internal
{
    internal class Grouping<TKey, TElement> : IGrouping<TKey, TElement>, IList<TElement>

Actually, this looks really familiar - if we're pulling in this from corefx can we link back and attribute accordingly?

Something like this:

/// adapted from System.Linq.Grouping from .NET Framework
/// source: https://github.com/dotnet/corefx/blob/b90532bc97b07234a7d18073819d019645285f1c/src/System.Linq/src/System/Linq/Grouping.cs#L64

I've just used the latest commit on master as the reference here, you might have used a different source.

---

Ix.NET/Source/System.Interactive.Async/Lookup.cs, line 1 [r3] (raw file):

using System;

As this is a new file, please use the new license header:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information. 

---

Ix.NET/Source/System.Interactive.Async/Lookup.cs, line 9 [r3] (raw file):

using System.Threading.Tasks;

namespace System.Linq.Internal

Same thing here - I'd like us to cite sources wherever appropriate for better visibility.

I gather this is coming from CoreFx, and so we should use System.Linq for the namespace to be consistent with upstream.

---

Ix.NET/Source/System.Interactive.Async/Strings.cs, line 10 [r3] (raw file):

        public static string NO_ELEMENTS = "Source sequence doesn't contain any elements.";
        public static string MORE_THAN_ONE_ELEMENT = "Source sequence contains more than one element.";
        public static string NOT_SUPPORTED = "NOT SUPPORTED";

Could we make this more user-friendly? cc @onovtny for feels

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

        }

        public static void Handle<T, R>(this Task<T> task, TaskCompletionSource<R> tcs, Action<T> success)

I'm not sure if external callers would have relied on this code (let alone should have), but given it is public static I'm hesitant to remove this right now. Feels @onovotny @bartdesmet?

---

Ix.NET/Source/Tests/AsyncTests.Aggregates.cs, line 21 [r3] (raw file):

    public partial class AsyncTests
    {
        private const int WaitTimeoutMs = 5000;

I'm not averse to putting timeouts here in the tests, especially where .Wait() is used, but I'd love to understand more about what you found while running the tests that necessitated this change...

---

Ix.NET/Source/Tests/AsyncTests.cs, line 24 [r3] (raw file):

        [Obsolete("Don't use this, use Assert.ThrowsAsync and await it", true)]
        public Task AssertThrows<E>(Func<Task> func)

I don't see any usages of this. Shall we clean this up?

---

Comments from Reviewable

[shiftkey]

@aelij thanks for contributing this! Don't worry about the volume of comments I've added, that's just me trying to be thorough and ensure nothing slips through the review process.

For the most part, the stuff I've raised is just cleanup jobs (prefixed with a 💄 emoji) but there's a few questions I have as well.

[clairernovotny]

Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 102 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

to ignore it for now here

So we should leave this as-is or undo this change?

Was this new or in there before, that's what I'm confused about. If it's new, then probably nuke it, otherwise, leave it as it's harmless and we'll clean up ifdef's later.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1299 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

This is an introduced checked here - is this just to be consistent or did you encounter something unexpected here during the migration?

Seems like this is logically the correct behavior though -- if we get past `int.MaxValue` it should throw, shouldn't it?

---

Ix.NET/Source/System.Interactive.Async/Grouping.cs, line 9 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

Actually, this looks really familiar - if we're pulling in this from corefx can we link back and attribute accordingly?

Something like this:

/// adapted from System.Linq.Grouping from .NET Framework
/// source: https://github.com/dotnet/corefx/blob/b90532bc97b07234a7d18073819d019645285f1c/src/System.Linq/src/System/Linq/Grouping.cs#L64

I've just used the latest commit on master as the reference here, you might have used a different source.

Yup, makes sense.

---

Ix.NET/Source/System.Interactive.Async/Lookup.cs, line 9 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

Same thing here - I'd like us to cite sources wherever appropriate for better visibility.

I gather this is coming from CoreFx, and so we should use System.Linq for the namespace to be consistent with upstream.

Agreed. I took this from CoreFX, but a link would be good here. In this case, we cannot use `System.Linq` as the namespace as `Lookup` is public there and it'd conflict. They don't expose the public ctor or methods we need to use, so I adapted their impl but made it internal. It's only returned to callers as an `ILookup` so it's functionally the same.

---

Ix.NET/Source/System.Interactive.Async/Strings.cs, line 10 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

Could we make this more user-friendly? cc @onovtny for feels

Up for any suggestions on this. Also will be easier to make into a ResX once back on MSBuild in the fall.

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

I'm not sure if external callers would have relied on this code (let alone should have), but given it is public static I'm hesitant to remove this right now. Feels @onovotny @bartdesmet?

I would recommend leaving it in with an `Obsolete` attribute added as a warning.

---

Ix.NET/Source/Tests/AsyncTests.cs, line 24 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

I don't see any usages of this. Shall we clean this up?

I left it there as a preventative measure as there's still the non-task overload that would work as an async void. This is there to catch accidental usages and fail.

---

Comments from Reviewable

[shiftkey]

Reviewed 6 of 13 files at r1, 9 of 15 files at r2, 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 102 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Was this new or in there before, that's what I'm confused about. If it's new, then probably nuke it, otherwise, leave it as it's harmless and we'll clean up ifdef's later.

It was introduced in 6180f06

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1299 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Seems like this is logically the correct behavior though -- if we get past int.MaxValue it should throw, shouldn't it?

Yeah, I'm fine with playing it safe here. No further context on this beyond when the code was OSSed in the first commit.

---

Ix.NET/Source/System.Interactive.Async/Strings.cs, line 10 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Up for any suggestions on this. Also will be easier to make into a ResX once back on MSBuild in the fall.

What about `"Specified method is not supported."` (taken from the default for the .NET Framework) for the message?

---

Ix.NET/Source/Tests/AsyncTests.cs, line 24 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

I left it there as a preventative measure as there's still the non-task overload that would work as an async void. This is there to catch accidental usages and fail.

👍 let's leave it in

---

Comments from Reviewable

[aelij]

@shiftkey I don't mind, although I would've done this in a separate PR (most of the 'lipsticks' existed in the original code).

Regarding the 'var' changes, would anyone mind if I added a ReSharper rule (in a committed .DotSettings file)?

---

Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Aggregates.cs, line 1623 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

💄 maybe inline this method as there are no other overloads?

The purpose of separating each operator to private methods is to preserve the semantics of the previous version - throw immediately and not asynchronously. If you take a look at the LINQ implementation (that uses compiler-generated iterators), you'll see the same thing.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 23 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

We don't seem to respect the provided CancellationToken inside the provided moveNext - do you think we should?

Sure, we could add `ct.ThrowIfCancellationRequested()`. I added this method because I needed a way to transform an enumerable to an async enumerable without creating thread-pool tasks.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1120 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

I think this is fine to leave here, as the old code using TaskCompletionSource meant that TrySetException wasn't going to throw the exception (but .Throw() here should).

We can also do what Roslyn does in such cases `throw ExceptionUtils.Unreachable` (which throws an exception saying "This program location is thought to be unreachable")

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1232 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

Can we document why this is an important scenario to catch and rethrow explicitly, rather than fall through to the onError?

I think it's self-evident. Cancellation has special handling throughout the code.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1299 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

Yeah, I'm fine with playing it safe here. No further context on this beyond when the code was OSSed in the first commit.

I added it because it exists in all other operators.

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

I would recommend leaving it in with an Obsolete attribute added as a warning.

This entire class is internal. We can definitely remove unused methods.

---

Ix.NET/Source/Tests/AsyncTests.Aggregates.cs, line 21 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

I'm not averse to putting timeouts here in the tests, especially where .Wait() is used, but I'd love to understand more about what you found while running the tests that necessitated this change...

When I transitioned the code to use await, it became apparent that Dispose() played a significant role here, and there were some cases where it didn't get called properly (bugs I introduced and fixed), but it made the tests hang, which was annoying.

---

Comments from Reviewable

[aelij]

Review status: all files reviewed at latest revision, 32 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 900 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

💄 do we need the == true bit here?

It's a nullable bool

---

Comments from Reviewable

[shiftkey]

@aelij 👍

---

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 23 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

Sure, we could add ct.ThrowIfCancellationRequested(). I added this method because I needed a way to transform an enumerable to an async enumerable without creating thread-pool tasks.

@onovotny thoughts? or just skip it?

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 900 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

It's a nullable bool

Oops, my bad. 👍

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1120 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

We can also do what Roslyn does in such cases throw ExceptionUtils.Unreachable (which throws an exception saying "This program location is thought to be unreachable")

@onovotny thoughts?

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

This entire class is internal. We can definitely remove unused methods.

@aelij good point!

---

Ix.NET/Source/Tests/AsyncTests.Aggregates.cs, line 21 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

When I transitioned the code to use await, it became apparent that Dispose() played a significant role here, and there were some cases where it didn't get called properly (bugs I introduced and fixed), but it made the tests hang, which was annoying.

@onovotny are we cool with this, at least until we get around to updating the tests to behave well with xUnit 2's infrastructure?

---

Comments from Reviewable

[clairernovotny]

There is already a solution level settings file for each project but that particular rule may not be set to always switch/use var. Updating that solution level setting for each is 👍 for me.

---

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Aggregates.cs, line 1623 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

The purpose of separating each operator to private methods is to preserve the semantics of the previous version - throw immediately and not asynchronously. If you take a look at the LINQ implementation (that uses compiler-generated iterators), you'll see the same thing.

Makes sense. This will get cleaner with C# 7 with inline functions as it's that exact behavior that's one of the goals of doing it.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 23 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

@onovotny thoughts? or just skip it?

I tend to think that `CancellationToken`'s should be honored if present. Seems like calling `ThrowIfCancellationRequested` is a good idea.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 102 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

It was introduced in 6180f06

got it. I'd say leave it as-is. That ifdef was for platforms that didn't have `IObserver`/`IObservable` in the BCL (Silverlight). Kind of a clean-as-we-go approach, though I can do a "big bang" cleanup too.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1120 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

@onovotny thoughts?

Not sure I have a strong opinion here. If it's the same situation as Roslyn, we can follow their lead.

---

Ix.NET/Source/System.Interactive.Async/Strings.cs, line 10 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

What about "Specified method is not supported." (taken from the default for the .NET Framework) for the message?

Fair enough, works for me :)

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

@aelij good point!

🔥

---

Ix.NET/Source/Tests/AsyncTests.Aggregates.cs, line 21 [r3] (raw file):

Previously, shiftkey (Brendan Forster) wrote…

@onovotny are we cool with this, at least until we get around to updating the tests to behave well with xUnit 2's infrastructure?

I'm definitely in favor of non-hanging tests, even if they're just exposing bugs we need to fix. Figuring out which test(s) hang is a time sink

---

Comments from Reviewable

[aelij]

Review status: all files reviewed at latest revision, 29 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 23 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

I tend to think that CancellationToken's should be honored if present. Seems like calling ThrowIfCancellationRequested is a good idea.

Following the discussion in #202 perhaps I should remove this method and add it in a different PR?

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 35 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

🔥

I've removed the Finally method (there was one use but I could convert it to a try..finally block).

---

Comments from Reviewable

[clairernovotny]

Review status: all files reviewed at latest revision, 28 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Conversions.cs, line 23 [r3] (raw file):

Previously, aelij (Eli Arbel) wrote…

Following the discussion in #202 perhaps I should remove this method and add it in a different PR?

Yeah, makes sense -- let's track this separately. I don't like the current behavior, but I'd like to get the rest of this in while we figure out the right answer to the other this.

---

Comments from Reviewable

[clairernovotny]

Reviewed 10 of 10 files at r4.
Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Creation.cs, line 80 [r4] (raw file):

                    using (ct.Register(stop))
                    {
                        return await moveNext(ct, tcs);

.ConfigureAwait(false)?

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 13 [r4] (raw file):

        public static readonly Task<bool> False = Task.FromResult(false);

        public static Task<T> Throw<T>(Exception exception)

Where is this still being used, and can we get rid of it?

---

Comments from Reviewable

[aelij]

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Creation.cs, line 80 [r4] (raw file):

Previously, onovotny (Oren Novotny) wrote…

.ConfigureAwait(false)?

Sigh. Serves me right for not supporting the ReSharper EAP for my **own** extension (ConfigureAwaitChecker). :)

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 13 [r4] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Where is this still being used, and can we get rid of it?

I looked into getting rid of it, but it's used in a few legitimate places (e.g. the `Throw` operator). This is a useful method - even .NET 4.6 added a `Task.FromException()`, but it's unavailable in the selected netstandard.

---

Comments from Reviewable

[shiftkey]

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Single.cs, line 1120 [r3] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Not sure I have a strong opinion here. If it's the same situation as Roslyn, we can follow their lead.

This interests me, but not really strongly enough to address it here. Let's leave this as-is.

---

Comments from Reviewable

[clairernovotny]

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Creation.cs, line 80 [r4] (raw file):

Previously, aelij (Eli Arbel) wrote…

Sigh. Serves me right for not supporting the ReSharper EAP for my own extension (ConfigureAwaitChecker). :)

Will happily install that extension once available for the EAP. I do wish ReSharper's EAP's didn't constantly break add-ins.

---

Ix.NET/Source/System.Interactive.Async/TaskExt.cs, line 13 [r4] (raw file):

Previously, aelij (Eli Arbel) wrote…

I looked into getting rid of it, but it's used in a few legitimate places (e.g. the Throw operator). This is a useful method - even .NET 4.6 added a Task.FromException(), but it's unavailable in the selected netstandard.

👍

---

Comments from Reviewable

[aelij]

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Creation.cs, line 80 [r4] (raw file):

Previously, onovotny (Oren Novotny) wrote…

Will happily install that extension once available for the EAP. I do wish ReSharper's EAP's didn't constantly break add-ins.

Hmm, how about installing the [Microsoft.ApiDesignGuidelines.Analyzers](https://www.nuget.org/packages/Microsoft.ApiDesignGuidelines.Analyzers/1.2.0-beta2) pack for this project? It has several useful diagnostics, including `CA2007` - Do not directly await a Task. This way we're not R#-dependent, and the warnings would show up in the CI build.

---

Comments from Reviewable

[clairernovotny]

Review status: all files reviewed at latest revision, 30 unresolved discussions.

---

Ix.NET/Source/System.Interactive.Async/AsyncEnumerable.Creation.cs, line 80 [r4] (raw file):

Previously, aelij (Eli Arbel) wrote…

Hmm, how about installing the Microsoft.ApiDesignGuidelines.Analyzers pack for this project? It has several useful diagnostics, including CA2007 - Do not directly await a Task. This way we're not R#-dependent, and the warnings would show up in the CI build.

Will take a look at that, thanks for the tip!

---

Comments from Reviewable

[shiftkey]

[clairernovotny]

@aelij Thanks for your hard work here!

[aelij]

🎉

@onovotny Please fix the ConfigureAwait() here. I was waiting for all the discussions to be resolved, and didn't realize you were going to merge it :)

Thanks!

[clairernovotny]

@aelij done

[aelij]

Thanks guys. It's been fun.

Now it's time to start thinking about #194 :)