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.

Sign up for GitHub

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

Jump to bottom

FanoutPublisherSink leaking actors on upstream failure #25796

Merged
merged 4 commits into from Mar 27, 2019
Merged

FanoutPublisherSink leaking actors on upstream failure #25796

merged 4 commits into from Mar 27, 2019

Conversation

johanandren
Copy link
Member

Fixes #25634

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Oct 15, 2018
@akka-ci
Copy link

akka-ci commented Oct 15, 2018

Test FAILed.

@johanandren johanandren force-pushed the wip-25634-FanoutPublisherSink-actor-leak-johanandren branch from e80b85e to df8bbaf Compare October 30, 2018 11:39
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Oct 30, 2018
@akka-ci
Copy link

akka-ci commented Oct 30, 2018

Test PASSed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Oct 30, 2018
@akka-ci
Copy link

akka-ci commented Oct 30, 2018

Test PASSed.

patriknw
patriknw approved these changes Mar 18, 2019
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM, let's move on

akka-stream-tests/src/test/scala/akka/stream/impl/FanoutProcessorSpec.scala Show resolved Hide resolved
akka-stream/src/main/scala/akka/stream/impl/FanoutProcessor.scala
primaryInputs.cancel()
primaryOutputs.error(e)
// Stopping will happen after flush
}
Copy link
Member

Choose a reason for hiding this comment

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

always scary to change/remove something that someone else has consciously added and even commented, but obviously there is a problem and no other test fails

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wonder if it was true at some point but other things changed around it perhaps. Or some assumption was wrong.

Copy link

@edwinchoi edwinchoi Mar 20, 2019 •
edited

Choose a reason for hiding this comment

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

With all due respect, I’m a bit surprised at the questions being raised here. If you take the comment at face value, you can easily disprove the claim. If it did happen to be that the behavior was changed due to an unintended consequence, the idea that the corner case would not have been documented seems flawed, unless you assume the second author was less diligent than the first.

For me, this issue caused the process to run out of memory in a non obvious way, with no workaround without a redesign. While I can appreciate the desire to be attentive to risks that can lead to unexpected regressions, but arbitrary risk aversion has costs. I think you guys created a great product with easy to grasp abstracions that clearly show a respect for the user experience, and I appreciate the thought and consideration you and your team put into it.

My issue is though that by investing a little more time in learning rxjava2 or monix, there are far less surprises.

Edit: fix bad edit
Edit #2: I appreciate your teams attentiveness to the issue. It’s not for a lack of appreciation or eagerness that I have limited my participation. Suffice it to say, the company I work for does not make it easy for me to contribute.

@patriknw patriknw requested a review from raboof March 18, 2019 13:18
@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Mar 20, 2019
@johanandren johanandren force-pushed the wip-25634-FanoutPublisherSink-actor-leak-johanandren branch from 6d3732c to a86abb3 Compare March 20, 2019 08:09
@akka-ci akka-ci added tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Mar 20, 2019
@akka-ci
Copy link

akka-ci commented Mar 20, 2019

Test PASSed.

1 similar comment
@akka-ci
Copy link

akka-ci commented Mar 20, 2019

Test PASSed.

chbatey
chbatey approved these changes Mar 27, 2019
View reviewed changes
Copy link
Member

@chbatey chbatey left a comment

Choose a reason for hiding this comment

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

LGTM

@johanandren johanandren merged commit 39c06c7 into akka:master Mar 27, 2019
@johanandren johanandren deleted the wip-25634-FanoutPublisherSink-actor-leak-johanandren branch March 27, 2019 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@viktorklang viktorklang viktorklang left review comments

@edwinchoi edwinchoi edwinchoi left review comments

@patriknw patriknw patriknw approved these changes

@chbatey chbatey chbatey approved these changes

@raboof raboof Awaiting requested review from raboof

Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@johanandren @akka-ci @viktorklang @patriknw @edwinchoi @chbatey