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

Complete MergeHub Sink gracefully on NormalShutdownException #7468

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

anpin
Copy link
Contributor

@anpin anpin commented Jan 14, 2025

Fixes #6931

Changes

Pattern match exception to avoid unnecessary errors logged upon Akka.IO streams failure

Checklist

For significant changes, please ensure that the following have been completed (delete if not relevant):

Latest dev Benchmarks

Include data from the relevant benchmark prior to this change here.

This PR's Benchmarks

Include data from after this change here.

@anpin
Copy link
Contributor Author

anpin commented Jan 14, 2025

Draft targets 1.5.30 tag since that is the version in my project. I can rebase to dev once we agree that this should be merged.

@Aaronontheweb
Copy link
Member

@anpin changes look good but this spec is failing (please check the build log):

System.AggregateException : One or more errors occurred. (Timeout (00:00:03) while waiting for messages. Only received 0/1 messages that matched filter [Error when Message contains "Upstream producer failed with exception"])
---- Timeout (00:00:03) while waiting for messages. Only received 0/1 messages that matched filter [Error when Message contains "Upstream producer failed with exception"]

Is that to be expected?

Copy link
Member

@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.

LGTM but please see my comment RE: test failrues

src/core/Akka.Streams.Tests/Dsl/HubSpec.cs Show resolved Hide resolved
src/core/Akka.Streams/Dsl/Hub.cs Show resolved Hide resolved
@Aaronontheweb
Copy link
Member

The other two tests that failed:

  1. LogFormatSpec should be addressed when we rebase on dev, which I have just done
  2. The TCP chain of echoes spec is a frequently racy unit test and is not impacted by this change, I think

@anpin
Copy link
Contributor Author

anpin commented Jan 21, 2025

Is that to be expected?

image

I don't know, seems to work fine on my end in Rider. Maybe FAKE on Windows somehow leads to a different result?

@Aaronontheweb
Copy link
Member

Give it a "run until failure" and let it cook for a little bit (like 10-15 minutes while you're doing something else) - I had to do that myself today for a couple of specs I was working on.

@Aaronontheweb
Copy link
Member

But otherwise you can mark this PR as "ready for review"

@anpin
Copy link
Contributor Author

anpin commented Jan 21, 2025

But otherwise you can mark this PR as "ready for review"

I''ll give it a second chance to fail tomorrow using your suggestion and will cleanup those comments in tests. Thank you @Aaronontheweb for looking into this!

@Aaronontheweb
Copy link
Member

@anpin thank you for your hard work and research on this issue!

@He-Pin
Copy link

He-Pin commented Jan 21, 2025

Nice find, will port this fix to Akka Scala too?

@anpin anpin marked this pull request as ready for review January 21, 2025 13:35
@anpin
Copy link
Contributor Author

anpin commented Jan 21, 2025

@He-Pin not a scala user myself, but you are welcome to do so if such an issue exists there

@He-Pin
Copy link

He-Pin commented Jan 21, 2025

I will try to port it to pekko, after this been merged, thanks. @anpin

@anpin
Copy link
Contributor Author

anpin commented Jan 21, 2025

LogFormatSpec should be addressed when we rebase on dev

@Aaronontheweb could you clarify what you meant by this?

I couldn't make those test fail in Rider on Linux. However I spotted a lot of error logs on linux in CI (for all PR's not this one in particular) and that the build script depends on outdated FAKE tooling, so my results are potentially not reproducible due to difference in environment used.

@anpin
Copy link
Contributor Author

anpin commented Jan 21, 2025

I poked a few of these failing tests locally
Screenshot From 2025-01-21 10-57-57

@Aaronontheweb
Copy link
Member

LogFormatSpec should be addressed when we rebase on dev

@Aaronontheweb could you clarify what you meant by this?

Mistake on my part - I thought there was an issue with the log formatting being broken in the version of Akka.NET (v1.5.30) you'd based your branch on. There might be a race condition in that test (very common and annoying in a test suite as large as ours.)

@Aaronontheweb
Copy link
Member

The FAKE build errors is mostly output from https://github.com/petabridge/Incrementalist - which can run into some solution analysis hiccups that aren't significant

Copy link
Member

@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.

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) January 21, 2025 19:10
@Aaronontheweb Aaronontheweb merged commit f80a2b1 into akkadotnet:dev Jan 21, 2025
12 checks passed
@Aaronontheweb Aaronontheweb modified the milestone: 1.5.36 Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IO-TCP logs unnecessary error message on flow termination
3 participants