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

Regression: LagomClientFactory ActorSystem binding on akka-remote port #1557

Closed
renatocaval opened this Issue Aug 28, 2018 · 12 comments

Comments

Projects
None yet
4 participants
@renatocaval
Member

renatocaval commented Aug 28, 2018

In #1256 we changed how we load the config that is then used to create the client's ActorSystem.

This has the undesired effect of causing that ActorSystem to try to bind on the remote port. This can only happen if the LagomClientFactory uses a classpath that contains the akka-remote jar.

The original intention of LagomClientFactory is to provide a Lagom client factory for non-Lagom applications that need to connect to Lagom application. However, it should be also possible to use it inside Lagom application, when the URL can't be looked up.

A possible simple solutions is to disable remote on the internal ActorSystem. There is no reason for having remote enabled here. This is a side-effect of using the client in an app with remote in the classpath.

@renatocaval renatocaval changed the title from Regression in LagomClientFactory to Regression: LagomClientFactory ActorSystem binding on akka-remote port Aug 28, 2018

@TimMoore

This comment has been minimized.

Member

TimMoore commented Aug 28, 2018

I'm wary of trying to override configuration settings on the internal ActorSystem. There are lots of things that could conflict. For example, by default Akka installs a JVM shutdown hook. If installed by two different actor systems, this could cause race conditions during shutdown. Chasing down everything that could possibly conflict and overriding it could be an endless game of whack-a-mole.

A better option would be to make something like LagomClientFactory available to Dependency Injection so that clients can be created dynamically. An easier alternative could be to provide an alternative creation method on the existing LagomClientFactory that takes an ActorSystem parameter.

@renatocaval

This comment has been minimized.

Member

renatocaval commented Aug 28, 2018

I considered the option of passing an ActorSystem to the create method, but there are other complications as well.

It's not only Lagom's ActorSystem that we should pass, but also the materializer. Then, we should also think about the close() method. I think we can end up by increasing the matrix of possible usages and therefore increase the chances of doing something wrong.

If that's something that is managed by DI, then users should not call the close() method.

If the users initialise it themselves, then there are two cases:

  1. did they pass an ActorSystem and Materializer? If so, close() method should not stop the ActorSystem.
  2. did they used it with internal ActorSystem, then close() method must behave differently.

About the JVM shutdown hook, I think it boils down to one simple assertion. We should not start an ActorSystem without controlling the configuration we are passing to it and that's exactly what we are doing here.

Maybe, we should just introduce a config namespace for it, eg:

lagom-client {
  akka {
  }
}

and provide a minimal config for the internal ActorSystem. That ActorSystem is expected to have its lifecycle attached to the client factory lifecycle, when the user closes the factory, the actor system is terminated. In this case, if should not have a JVM shutdown hook, for instance. Unless explicitly configured by user.

@aklikic

This comment has been minimized.

aklikic commented Aug 28, 2018

+1

@aklikic

This comment has been minimized.

aklikic commented Aug 28, 2018

Hi,

does it make sense to have two versions of factory? One for use within Lagom service and one in non-lagom services.

Br,
Alan

@renatocaval

This comment has been minimized.

Member

renatocaval commented Aug 28, 2018

I don't think that my proposal to have a namespaced config for the internal actor system is a good idea. It will bring back the issue that was solved by #1256.

@renatocaval

This comment has been minimized.

Member

renatocaval commented Aug 28, 2018

@aklikic, I don't think we should have two versions of it. An alternative create method should suffice.

@TimMoore I will experiment with the easiest solution for now, which is add an alternative method and different strategies for close() method. We can later evaluate if we want to have it also available via injection.

@aklikic

This comment has been minimized.

aklikic commented Aug 28, 2018

@renatocaval just to confirm that providing Lagom's actor system and materialization works ok. So currently we are using our implementation of factory
Are there any plans of porting netty to akka http?

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Aug 28, 2018

Are there any plans of porting netty to akka http?

This is planned, no ETA though.

@renatocaval

This comment has been minimized.

Member

renatocaval commented Aug 28, 2018

@TimMoore I have some extra thoughts about explicitly disabling configs.

I've been working on an overloaded method that gets an external ActorSystem, so we can use Lagom's one when using the LagomClientFactory from inside a Lagom application.

But that doesn't exclude the possibility to have LagomClientFactory being used in a non-Lagom applications that happen to have already an ActorSystem running.

I agree that this can become an endless game, but I don't think we have much of a choice here. We are starting and stopping an ActorSystem without controlling its configuration and the environment where it will start.

Of course, once we have the new method, we can always recommend users to use it in case they have an ActorSystem already initialised, but we can't enforce it.

ignasi35 added a commit that referenced this issue Sep 18, 2018

Extends LagomClientFactory to use external ActorSystem (#1560)
Fixes #1557 

Overloads LagomClientFactory to allow injecting an external ActorSytem (avoid using two actor systems in a single process)

@ignasi35 ignasi35 reopened this Sep 18, 2018

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 18, 2018

#1560 fixes this issue in master but a manual backport to 1.4.x will be necessary.

@ignasi35

This comment has been minimized.

Member

ignasi35 commented Sep 18, 2018

#1591 fixes 1.4.x

@renatocaval

This comment has been minimized.

Member

renatocaval commented Sep 27, 2018

#1591 is merged, closing this.

erip added a commit to erip/lagom that referenced this issue Sep 30, 2018

Extends LagomClientFactory to use external ActorSystem (lagom#1560)
Fixes lagom#1557 

Overloads LagomClientFactory to allow injecting an external ActorSytem (avoid using two actor systems in a single process)

@TimMoore TimMoore added this to the Lagom 1.4.9 milestone Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment