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

Add customized NPE for null actor system #23580

Merged
merged 2 commits into from Sep 4, 2017
Merged

Conversation

wsargent
Copy link
Contributor

@wsargent wsargent commented Aug 30, 2017

Change

Add a customized NPE that indicates that the passed in actor system was null here. IllegalArgumentException would also be appropriate.

Rationale

In the HttpApp, there are various hooks that are provided for a running server:

http://doc.akka.io/docs/akka-http/current/scala/http/routing-dsl/HttpApp.html#providing-your-own-actor-system

However, the access to the actor system is controlled by systemReference, which can be null. If you try accessing CoordinatedShutdown(systemReference.get()) you'll get

[error] Caused by: java.lang.NullPointerException
[error] at akka.actor.ExtensionId.apply(Extension.scala:77)
[error] at akka.actor.ExtensionId.apply$(Extension.scala:77)
[error] at akka.actor.CoordinatedShutdown$.apply(CoordinatedShutdown.scala:34)

Which does not indicate that a null actor system was passed in unless you look at the source code.

@akka-ci
Copy link

akka-ci commented Aug 30, 2017

Can one of the repo owners verify this patch?

@raboof
Copy link
Member

raboof commented Aug 30, 2017

OK TO TEST

@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 Aug 30, 2017
@akka-ci
Copy link

akka-ci commented Aug 30, 2017

Test PASSed.

@@ -74,7 +74,9 @@ trait ExtensionId[T <: Extension] {
/**
* Returns an instance of the extension identified by this ExtensionId instance.
*/
def apply(system: ActorSystem): T = system.registerExtension(this)
def apply(system: ActorSystem): T = {
java.util.Objects.requireNonNull(system, "Null system!").registerExtension(this)
Copy link
Member

Choose a reason for hiding this comment

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

system must not be null!?

@ktoso
Copy link
Member

ktoso commented Aug 31, 2017

LGTM, proposed slight rewording (can make the change if you think it's ok)

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.

LGTM with ktosos wording

@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 Sep 1, 2017
@akka-ci
Copy link

akka-ci commented Sep 1, 2017

Test PASSed.

Copy link
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

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

Generally I'd say this is the responsibility of the caller, but this might be a relatively common case and it doesn't cost anything to produce a better message here, so 👍

@raboof raboof merged commit b600c51 into akka:master Sep 4, 2017
@wsargent wsargent deleted the patch-3 branch September 5, 2017 01:59
@ktoso ktoso added this to the 2.5.5 milestone Sep 5, 2017
@ktoso
Copy link
Member

ktoso commented Sep 5, 2017

Assigned milestone

lucianenache pushed a commit to lucianenache/akka that referenced this pull request Nov 6, 2017
* Add customized NPE for null actor system

* "system must not be null!"
lucianenache pushed a commit to lucianenache/akka that referenced this pull request Nov 9, 2017
* Add customized NPE for null actor system

* "system must not be null!"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants