Netflix / zuul

Zuul is a gateway service that provides dynamic routing, monitoring, resiliency, security, and more.
Apache License 2.0
13.44k stars 2.37k forks source link

[Zuul2][test] PassportStateServerHandler: IllegalStateException: registry already set #700

Open pcabot opened 4 years ago

pcabot commented 4 years ago

NOTE: not sure if this is a bug or a misuse of the zuul server.

Affected Version: com.netflix.zuul:zuul-core:2.1.6

Context: Running instances of zuul server part of integration tests (using Junit).

Steps:

Expected Result: No errors (same behavior as with version 2.1.3)

What happened instead ? Using version 2.1.6, the following PassportStateServerHandler spectator related precondition exception is now thrown when the second test method is executed (i.e. when the second zuul server is started):

2020-01-07 16:17:33,000 WARN  io.netty.channel.ChannelInitializer [Salamander-ClientToZuulWorker-0] Failed to initialize a channel. Closing: [id: 0x0dc14a75, L:/127.0.0.1:57176 - R:/127.0.0.1:57191]
java.lang.IllegalStateException: registry already set
    at com.google.common.base.Preconditions.checkState(Preconditions.java:508)
    at com.netflix.zuul.netty.insights.PassportStateServerHandler.setRegistry(PassportStateServerHandler.java:52)
    at com.netflix.zuul.netty.server.BaseZuulChannelInitializer.addPassportHandler(BaseZuulChannelInitializer.java:226)
    at com.netflix.zuul.netty.server.ZuulServerChannelInitializer.initChannel(ZuulServerChannelInitializer.java:59)
    at io.netty.channel.ChannelInitializer.initChannel(ChannelInitializer.java:129)
    at io.netty.channel.ChannelInitializer.handlerAdded(ChannelInitializer.java:112)
    at io.netty.channel.AbstractChannelHandlerContext.callHandlerAdded(AbstractChannelHandlerContext.java:964)
    at io.netty.channel.DefaultChannelPipeline.callHandlerAdded0(DefaultChannelPipeline.java:613)
    at io.netty.channel.DefaultChannelPipeline.access$100(DefaultChannelPipeline.java:46)
    at io.netty.channel.DefaultChannelPipeline$PendingHandlerAddedTask.execute(DefaultChannelPipeline.java:1475)
    at io.netty.channel.DefaultChannelPipeline.callHandlerAddedForAllHandlers(DefaultChannelPipeline.java:1127)
    at io.netty.channel.DefaultChannelPipeline.invokeHandlerAddedIfNeeded(DefaultChannelPipeline.java:654)
    at io.netty.channel.AbstractChannel$AbstractUnsafe.register0(AbstractChannel.java:503)
    at io.netty.channel.AbstractChannel$AbstractUnsafe.access$200(AbstractChannel.java:416)
    at io.netty.channel.AbstractChannel$AbstractUnsafe$1.run(AbstractChannel.java:475)
    at io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:163)
    at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:510)
    at io.netty.channel.kqueue.KQueueEventLoop.run(KQueueEventLoop.java:293)
    at io.netty.util.concurrent.SingleThreadEventExecutor$6.run(SingleThreadEventExecutor.java:1050)
    at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
    at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
    at java.lang.Thread.run(Thread.java:748)

Analysis: The exception seems to be thrown by a precondition related to a static registry field used for analytics purposes (i.e. spectator). So, this new check makes it impossible to instantiate multiple zuul servers within the same JVM instance. Not sure if it ever was. May be, it is just not supported.

I was wondering if you had any workaround to suggest for running zuul instances in the context of integration tests? I am thinking of using reflection to reset the registry to null at the beginning of each test, since spectator is not really important in my integration test anyway. Any thoughts ?

carl-mastrangelo commented 4 years ago

Ideally the spectactor code would be replace with the later version, which doesn't do static init. I don't think it's easy to upgrade this though.

I wouldn't say your use case is supported, but I don't see any reason to prevent it.

pcabot commented 4 years ago

Ok, thanks for the info. That's what I thought: the static context makes it hard to use in tests. Until spectator gets upgraded to a non static version, I will use the workaround through reflection.