Log when global IoExecutor or Executor instance gets closed
#1337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Motivation: Global instance of `IoExecutor` and `Executor` is shared across all client and server instances by default (unless users provide custom executors). Closing them may affect all other clients and servers that depends on them. The only legit way of closing global executors if when everything else that depends on it is already closed. We should notify users if they close global instances. Modifications: - Wrap global `IoExecutor` and `Executor` instances and log when users subscribe to their `closeAsync()` or `closeAsyncGracefully()` methods; Result: Users who closed global `IoExecutor` or `Executor` in incorrect order and debug issues caused by this closure will see a debug message log that will point to the root cause.
tkountis
approved these changes
Jan 27, 2021
There was a problem hiding this comment.
Changes look good 🚀
Build failures highlight that there is an expectation that the IoExecutor is also an EventLoopAwareNettyIoExecutor. This begs the question whether we should enforce this check during compile-time when we construct the DnsClient?
java.lang.ExceptionInInitializerError
at io.servicetalk.http.netty.GlobalDnsServiceDiscoverer.globalDnsServiceDiscoverer(GlobalDnsServiceDiscoverer.java:49)
at io.servicetalk.http.netty.DefaultSingleAddressHttpClientBuilder.forHostAndPort(DefaultSingleAddressHttpClientBuilder.java:177)
at io.servicetalk.http.netty.HttpClients.forSingleAddress(HttpClients.java:136)
at io.servicetalk.http.router.jersey.AbstractNonParameterizedJerseyStreamingHttpServiceTest.initServerAndClient(AbstractNonParameterizedJerseyStreamingHttpServiceTest.java:160)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:566)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
at org.junit.internal.runners.statements.RunBefores.invokeMethod(RunBefores.java:33)
at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:24)
at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
at org.mockito.internal.junit.JUnitRule$1.evaluateSafely(JUnitRule.java:57)
at org.mockito.internal.junit.JUnitRule$1.evaluate(JUnitRule.java:48)
at org.junit.rules.ExpectedException$ExpectedExceptionStatement.evaluate(ExpectedException.java:258)
at io.servicetalk.concurrent.internal.ServiceTalkTestTimeout$TimeoutStatement$CallableStatement.call(ServiceTalkTestTimeout.java:170)
at io.servicetalk.concurrent.internal.ServiceTalkTestTimeout$TimeoutStatement$CallableStatement.call(ServiceTalkTestTimeout.java:162)
at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: java.lang.IllegalArgumentException: Incompatible IoExecutor: io.servicetalk.transport.netty.internal.GlobalExecutionContext$GlobalIoExecutor@512576ef. Not a netty based IoExecutor.
at io.servicetalk.transport.netty.internal.EventLoopAwareNettyIoExecutors.toEventLoopAwareNettyIoExecutor(EventLoopAwareNettyIoExecutors.java:43)
at io.servicetalk.dns.discovery.netty.DefaultDnsClient.<init>(DefaultDnsClient.java:135)
at io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.build(DefaultDnsServiceDiscovererBuilder.java:266)
at io.servicetalk.dns.discovery.netty.DefaultDnsServiceDiscovererBuilder.buildARecordDiscoverer(DefaultDnsServiceDiscovererBuilder.java:201)
at io.servicetalk.http.netty.GlobalDnsServiceDiscoverer$HostAndPortClientInitializer.<clinit>(GlobalDnsServiceDiscoverer.java:64)```
|
@tkountis thanks! |
bondolo
pushed a commit
to bondolo/servicetalk
that referenced
this pull request
Jan 29, 2021
…e#1337) Motivation: Global instances of `IoExecutor` and `Executor` are shared across all client and server instances by default (unless users provide custom executors). Closing them may affect all other clients and servers that depend on them. The only legit way of closing global executors is when everything else that depends on it is already closed. We should notify users if they close global instances. Modifications: - Wrap global `IoExecutor` and `Executor` instances and log when users subscribe to their `closeAsync()` or `closeAsyncGracefully()` methods; - Change `NettyIoExecutors#createIoExecutor` methods return type to `EventLoopAwareNettyIoExecutor`; Result: Users who closed global `IoExecutor` or `Executor` in incorrect order and debug issues caused by this closure will see a debug message log that will point to the root cause.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
Global instances of
IoExecutorandExecutorare shared across all clientand server instances by default (unless users provide custom executors).
Closing them may affect all other clients and servers that depend on them.
The only legit way of closing global executors is when everything else that
depends on it is already closed. We should notify users if they close
global instances.
Modifications:
IoExecutorandExecutorinstances and log when userssubscribe to their
closeAsync()orcloseAsyncGracefully()methods;NettyIoExecutors#createIoExecutormethods return type toEventLoopAwareNettyIoExecutor;Result:
Users who closed global
IoExecutororExecutorin incorrect order and debugissues caused by this closure will see a debug message log that will point to
the root cause.