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

Ensure NPE is always through when VirtualProcessor.onError(null) is invoked #25311

Merged
merged 2 commits into from
Jul 11, 2018
Merged

Ensure NPE is always through when VirtualProcessor.onError(null) is invoked #25311

merged 2 commits into from
Jul 11, 2018

Conversation

jroper
Copy link
Contributor

@jroper jroper commented Jul 5, 2018

This fix is similar to #24749, fixing a spec violation bug that was introduced in #24722.

I've rearranged the code to ensure that hopefully it never regresses again, instead of having to check if the exception was null in every case, it now simply does it at the end, assuming it successfully reaches the end.

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Jul 5, 2018
jroper
jroper commented Jul 5, 2018
View reviewed changes
akka-stream/src/main/scala/akka/stream/impl/StreamLayout.scala Outdated
@@ -259,14 +256,17 @@ import scala.util.control.NonFatal
case est @ Establishing(_, false, OptionVal.None) ⇒
if (VirtualProcessor.Debug) println(s"VirtualPublisher#$hashCode($est).onError(${t.getMessage}), loop")
if (!compareAndSet(est, est.copy(onErrorBuffered = OptionVal.Some(ex)))) rec(ex)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is where the actual bug being fixed was - if establishing, and the error was successfully buffered, then an NPE wasn't thrown if error was null.

@jroper jroper force-pushed the throw-npe branch 2 times, most recently from ca00286 to eb0bcbe Compare July 5, 2018 08:12
@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 validating PR is currently being validated by Jenkins labels Jul 5, 2018
@akka-ci
Copy link

akka-ci commented Jul 5, 2018

Test PASSed.

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

akka-ci commented Jul 5, 2018

Test PASSed.

johanandren
johanandren reviewed Jul 5, 2018
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good. Could you add a test or two (I'm thinking Subscriber and null states should be easy to test) also, since it doesn't seem to be covered by the TCK?

johanandren
johanandren approved these changes Jul 5, 2018
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

Oh wait, the actual regression was for Establishing and that would likely be impossible to reproduce repeatedly. NVM. LGTM!

@jroper
Copy link
Contributor Author

jroper commented Jul 5, 2018

Yeah, I don't think the case is practical to reproduce, though an interesting thing is, I'm pretty sure I upgraded to Akka Streams 2.5.12 (where the regression was introduced), and didn't encounter the issue, but then when I upgraded to 2.5.13, it now causes tests to fail more often than not. The thing that is reproducing this is the Akka Streams implementation of MicroProfile Reactive Streams Operators, which has a TCK that pretty much throws everything it can at the Reactive Streams TCK, for example here's the verification for the limit operator (equivalent to Akka Streams drop, called limit because it's following the JDK8 java.util.streams API naming):

https://github.com/eclipse/microprofile-reactive/blob/master/streams/tck/src/main/java/org/eclipse/microprofile/reactive/streams/tck/LimitStageVerification.java

You can see at the bottom it provides a Reactive Streams TCK IdentityProcessorVerification against a .limit(Long.MAX_VALUE) processor.

In #24749 a test was introduced that exercised at least one of the paths in this code, and I'm sure there are other tests that exercise other parts - it's probably just luck that the existing tests haven't been tripped by this yet.

@jroper
Copy link
Contributor Author

jroper commented Jul 5, 2018

Ok, I just ran it against 2.5.12, it definitely exhibits in 2.5.12.

@patriknw patriknw added the 2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding label Jul 9, 2018
patriknw
patriknw approved these changes Jul 10, 2018
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, with an additional improvement of the debug lines

akka-stream/src/main/scala/akka/stream/impl/StreamLayout.scala Outdated
@@ -240,16 +240,13 @@ import scala.util.control.NonFatal
case null ⇒
if (VirtualProcessor.Debug) println(s"VirtualPublisher#$hashCode(null).onError(${t.getMessage}) -> ErrorPublisher")
Copy link
Member

Choose a reason for hiding this comment

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

those debug lines are never enabled in released code, but for correctness the t.getMessage should be changed to ex.getMessage to avoid possible NPE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

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

akka-ci commented Jul 10, 2018

Test PASSed.

@johanandren johanandren merged commit fdcfa9d into akka:master Jul 11, 2018
@johanandren johanandren modified the milestone: 2.5.14 Jul 11, 2018
@johanandren
Copy link
Member

Fixes #25339

@jroper jroper deleted the throw-npe branch July 23, 2018 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patriknw patriknw patriknw approved these changes

@johanandren johanandren johanandren approved these changes

Assignees
No one assigned
Labels
2 - pick next Used to mark issues which are next up in the queue to be worked on. The tag is non-binding 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
4 participants
@jroper @akka-ci @johanandren @patriknw