Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Akka.TestKit: remove ConfigureAwait(false) from internal synchronous TestKit methods and fix Watch / Unwatch bugs #7037

Merged

Conversation

Aaronontheweb
Copy link
Member

@Aaronontheweb Aaronontheweb commented Jan 4, 2024

Changes

close #7038
close #7064

Removed all internal usages of ConfigureAwait(false) from synchronous Akka.TestKit method implementations - this should help reduce the incidences of mysterious racy unit tests.

@Aaronontheweb Aaronontheweb changed the title Resolve racy Akka.Persistence.TestKit.Tests [WIP] Akka.TestKit: remove ConfigureAwait(false) from internal synchronous TestKit methods Jan 4, 2024
@Aaronontheweb
Copy link
Member Author

This innocuous test is still failing, even after these changes, so it makes me think there's more async funk going on inside the TestKit that isn't completely resolved yet:

[Fact]
public async Task TestJournal_PersistAll_should_only_count_each_event_exceptions_once()
{
var probe = CreateTestProbe();
await WithJournalWrite(write => write.Pass(), async () =>
{
var actor = ActorOf(() => new TestActor2(probe));
Watch(actor);
var command = new WriteMessage();
actor.Tell(command, actor);
await probe.ExpectMsgAsync<RecoveryCompleted>();
await probe.ExpectMsgAsync<Done>();
await probe.ExpectMsgAsync<Done>();
await probe.ExpectNoMsgAsync(3000);
});
}
}

@Aaronontheweb
Copy link
Member Author

@akkadotnet/core not going to include this in Akka.NET v1.5.15 - too much of a hot potato and it needs time for lots of subsequent CI/CD runs to validate it before we roll out the "mission accomplished!" banner to our users

*
* "Nuke the site from orbit. It's the only way to be sure."
*/
taskCompletionSource.Task.Wait(RemainingOrDefault);
Copy link
Member Author

Choose a reason for hiding this comment

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

Nasty, decade old bug in the TestKit - Watch and Unwatch have always been secretly asynchronous, which is why tests like this one failed:

[Fact]
        public async Task Routers_in_general_must_evict_terminated_routees()
        {
            var router = Sys.ActorOf(new RoundRobinPool(2).Props(Props.Create<Echo>()));
            router.Tell("");
            router.Tell("");

            var c1 = await ExpectMsgAsync<IActorRef>();
            var c2 = await ExpectMsgAsync<IActorRef>();

            Watch(router);
            Watch(c2);
            Sys.Stop(c2);
            (await ExpectTerminatedAsync(c2)).ExistenceConfirmed.Should().BeTrue();

            // it might take a while until the Router has actually processed the Terminated message
            await AwaitConditionAsync(async () =>
            {
                router.Tell("");
                router.Tell("");

                var res = await ReceiveWhileAsync(100.Milliseconds(), x =>
                {
                    if (x is IActorRef actorRef)
                        return actorRef;

                    return null;
                }, msgs: 2).ToListAsync();

                return res.Count == 2 && res.All(c => c.Equals(c1));
            });

            Sys.Stop(c1);
            (await ExpectTerminatedAsync(router)).ExistenceConfirmed.Should().BeTrue();
        }

Failed on (await ExpectTerminatedAsync(c2)).ExistenceConfirmed.Should().BeTrue();, which can only mean the actor was dead before the TestActor had a chance to go through.

/// </summary>
/// <param name="actorToWatch">TBD</param>
public Watch(IActorRef actorToWatch) { _actorToWatch = actorToWatch; }
public Watch(IActorRef actorToWatch, TaskCompletionSource<bool> watchCompleted)
Copy link
Member Author

Choose a reason for hiding this comment

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

Messaging fixes to support the Watch and Unwatch raciness.

Copy link
Member Author

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

Left some comments about my Watch and Unwatch fixes.

src/core/Akka.TestKit/Internal/InternalTestActor.cs Outdated Show resolved Hide resolved
@Aaronontheweb Aaronontheweb added akka-testkit Akka.NET Testkit issues api-change labels Jan 17, 2024
@Aaronontheweb Aaronontheweb changed the title Akka.TestKit: remove ConfigureAwait(false) from internal synchronous TestKit methods Akka.TestKit: remove ConfigureAwait(false) from internal synchronous TestKit methods and fix Watch / Unwatch bugs Jan 17, 2024
Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

I have some concern on the Watch and Unwatch implementation and also how sync over async is implemented

src/core/Akka.TestKit/TestKitBase.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestProbe.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase_Receive.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase_Receive.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase_Receive.cs Outdated Show resolved Hide resolved
src/core/Akka.TestKit/TestKitBase_Receive.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@Arkatufus Arkatufus left a comment

Choose a reason for hiding this comment

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

LGTM

@Arkatufus Arkatufus enabled auto-merge (squash) January 17, 2024 20:56
@Aaronontheweb Aaronontheweb merged commit 3fb9247 into akkadotnet:dev Jan 17, 2024
8 of 12 checks passed
@Aaronontheweb Aaronontheweb deleted the akka-persistence-testkit-tests branch January 17, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants