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

Small TCK fix in cancelled sink #24749

Merged
merged 2 commits into from
Mar 19, 2018
Merged

Small TCK fix in cancelled sink #24749

merged 2 commits into from
Mar 19, 2018

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Mar 19, 2018 •
edited

When the sink is cancelled immediately (as Sink.cancelled does), if onError(null) is called, Akka still has to throw an NPE, which it wasn't doing.

Not sure if the TCK test added is ok - the cancelled sink never signals demand so I skipped the three specs that require that. That leaves 8 rules that this test still validates.

ktoso
ktoso approved these changes Mar 19, 2018
View reviewed changes
Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

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

Looks good, we even had a comment there explaining the need to throw that NPE but seems we didn't; thanks

The test looks fine

akka-stream/src/main/scala/akka/stream/impl/StreamLayout.scala Outdated
@@ -186,6 +186,9 @@ import scala.util.control.NonFatal
case Inert ⇒ // nothing to be done
case _ ⇒ ErrorPublisher(ex, "failed-VirtualProcessor").subscribe(s)
}
case _ if t == null ⇒
// cancelled before onError(null), must throw NPE, rule 2:13
Copy link
Member

Choose a reason for hiding this comment

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

2.13 please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had 2:13 because I copied the comment from a previous line, but I've updated both now.

ktoso
ktoso reviewed Mar 19, 2018
View reviewed changes
akka-stream/src/main/scala/akka/stream/impl/StreamLayout.scala Outdated
@@ -186,6 +186,9 @@ import scala.util.control.NonFatal
case Inert ⇒ // nothing to be done
case _ ⇒ ErrorPublisher(ex, "failed-VirtualProcessor").subscribe(s)
}
case _ if t == null ⇒
// cancelled before onError(null), must throw NPE, rule 2:13
throw ex
Copy link
Member

@ktoso ktoso Mar 19, 2018 •
edited

Choose a reason for hiding this comment

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

Hmm, might be good to throw ReactiveStreamsCompliance.exceptionMustNotBeNullException instead so it's more explanatory than a raw NPE?

Copy link
Member

Choose a reason for hiding this comment

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

nevermind, just noticed the line below which github had folded, it's all good

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Mar 19, 2018
@ktoso
Copy link
Member

ktoso commented Mar 19, 2018

Thanks, LGTM :) weird that we had the : style there... Thanks for fixing

@ktoso
Copy link
Member

ktoso commented Mar 19, 2018

To be merged once validation passed

@jroper
Copy link
Contributor Author

jroper commented Mar 19, 2018 •
edited

There's still actually one case that does't throw the NPE, not sure if you want me to fix it as well or not - it's the case where subscribe has been called, but onSubscribe hasn't been called yet. In that case, the invocation of onError is a spec violation regardless of whether the error is null or not, so it's probably not a big deal whether we throw or not. But then, the case where neither subscribe nor onSubscribe has been invoked is also a violation for the same reason, and it does rethrow the NPE.

@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 19, 2018
@akka-ci
Copy link

akka-ci commented Mar 19, 2018

Test PASSed.

@akka-ci akka-ci added the tested PR that was successfully built and tested by Jenkins label Mar 19, 2018
@akka-ci
Copy link

akka-ci commented Mar 19, 2018

Test PASSed.

@ktoso
Copy link
Member

ktoso commented Mar 19, 2018

Hmm, yeah that's a weird case. I guess we should throw then as well, if someone does such incorrect invocation; let's have a look at it in a separate PR though? I'll merge this one as it's clear cut and a nice improvement -- thanks!

@ktoso ktoso merged commit f1a5679 into akka:master Mar 19, 2018
@ktoso ktoso added this to the 2.5.12 milestone Mar 19, 2018
@jroper jroper deleted the cancel-subscriber-tck branch July 5, 2018 07:51
@jroper
Copy link
Contributor Author

jroper commented Jul 5, 2018

This fix was regressed in #24722.

jroper added a commit to jroper/akka that referenced this pull request Jul 5, 2018
@jroper
Ensure NPE is always through when VirtualProcessor.onError(null) is i…
10cccc9 
…nvoked

This fix is similar to akka#24749, fixing a spec violation bug that was
introduced in akka#24722.
@jroper jroper mentioned this pull request Jul 5, 2018
Merged
jroper added a commit to jroper/akka that referenced this pull request Jul 5, 2018
@jroper
Ensure NPE is always through when VirtualProcessor.onError(null) is i…
ca00286 
…nvoked

This fix is similar to akka#24749, fixing a spec violation bug that was
introduced in akka#24722.
jroper added a commit to jroper/akka that referenced this pull request Jul 5, 2018
@jroper
Ensure NPE is always through when VirtualProcessor.onError(null) is i…
eb0bcbe 
…nvoked

This fix is similar to akka#24749, fixing a spec violation bug that was
introduced in akka#24722.
jroper added a commit to jroper/akka that referenced this pull request Jul 10, 2018
@jroper
Ensure NPE is always through when VirtualProcessor.onError(null) is i…
36eee09 
…nvoked

This fix is similar to akka#24749, fixing a spec violation bug that was
introduced in akka#24722.
johanandren pushed a commit that referenced this pull request Jul 11, 2018
@jroper @johanandren
Ensure NPE is always through when VirtualProcessor.onError(null) is i…
fdcfa9d 
…nvoked (#25311)

* Ensure NPE is always through when VirtualProcessor.onError(null) is invoked
This fix is similar to #24749, fixing a spec violation bug that was
introduced in #24722.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@jroper @ktoso @akka-ci