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

Global resource cleanup slow on Mac OS #860

Closed
sbrannen opened this issue Sep 23, 2019 · 10 comments
Closed

Global resource cleanup slow on Mac OS #860

sbrannen opened this issue Sep 23, 2019 · 10 comments
Labels
good first issue Ideal for a new contributor, we'll help type/enhancement A general enhancement
Milestone

Comments

@sbrannen
Copy link

Overview

When using global HTTP resources for event loops and pooling on Mac OS, cleanup of those resources takes considerably longer on Mac OS than it does on Linux.

For example, after this commit in the Spring Framework, the WebClientDataBufferAllocatingTests test class began taking approximately 120 seconds instead of 2-3 seconds to complete.

After disabling the use of global resources in spring-projects/spring-framework@f0e160f#diff-cb71838b5a4c0062d77075023316ea9c, the WebClientDataBufferAllocatingTests test class once again completes in 2-3 seconds.

Based on a tip from a colleague that including a dependency on io.netty:netty-transport-native-kqueue:4.1.39.Final:osx-x86_64 might help, I tried that in the spring-webflux project, but that does not appear to make a difference.

Expected Behavior

The time taken for blocking global resource cleanup on Mac OS is comparable to cleanup on Linux.

Actual Behavior

The time taken for blocking global resource cleanup on Mac OS is considerably slower than on Linux.

Steps to Reproduce

Execute the WebClientDataBufferAllocatingTests test class with this.factory.setUseGlobalResources(false); commented out in the setUp() method.

Your Environment

  • Reactor version: Dysprosium-BUILD-SNAPSHOT
  • Netty version: 4.1.39.Final
  • JVM version: Oracle JDK 1.8.0 update 221
  • OS and version: Mac OS / 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64
@sbrannen sbrannen added status/need-triage A new issue that still need to be evaluated as a whole type/bug A general bug labels Sep 23, 2019
@violetagg violetagg removed the status/need-triage A new issue that still need to be evaluated as a whole label Sep 23, 2019
@violetagg
Copy link
Member

@sbrannen There is a problem with the test

When you initialise the WebClient, actually you still don't know the bufferFactory, so you do not create a WebClient with custom ConnectionProvider and LoopResources, but with the global ones. At the end, you dispose ConnectionProvider and LoopResources that have never been used and because of that their disposal finishes immediately.

Here is the code, you enter in the else branch and not the if branch.
https://github.com/spring-projects/spring-framework/blob/f0e160fc67d276d22b2908b6421351feeba7f492/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/WebClientDataBufferAllocatingTests.java#L76-L85

When we dispose the event loop group we invoke
https://netty.io/4.1/api/io/netty/util/concurrent/EventExecutorGroup.html#shutdownGracefully--
This default implementation is with 2s quiet period and 15s timeout

@violetagg violetagg added for/user-attention This issue needs user attention (feedback, rework, etc...) and removed type/bug A general bug labels Sep 23, 2019
@violetagg
Copy link
Member

The log below is an execution on Ubuntu - again 2s quiet period

^A^B^Cq21:47:22.105 [Test worker] DEBUG o.s.w.r.f.c.ExchangeFunctions - [4288649a] HTTP GET http://localhost:42291$
^@^B^CFSep 23, 2019 9:47:22 PM okhttp3.mockwebserver.MockWebServer$3 execute
^@^B^C:INFO: MockWebServer[42291] starting to accept connections
^A^B^Cj21:47:22.142 [reactor-http-epoll-1] DEBUG o.s.w.r.f.c.ExchangeFunctions - [4288649a] Response 201 CREATED
^@^B^CPSep 23, 2019 9:47:22 PM okhttp3.mockwebserver.MockWebServer$4 processOneRequest
^@^B^C_INFO: MockWebServer[42291] received request: GET /json HTTP/1.1 and responded: HTTP/1.1 201 OK
^A^B^C\21:47:22.155 [Test worker] INFO  r.M.D.2 - onSubscribe([Fuseable] MonoWhen.WhenCoordinator)
^A^B^C>21:47:22.158 [Test worker] INFO  r.M.D.2 - request(unbounded)
^A^B^CD21:47:24.366 [globalEventExecutor-1-2] INFO  r.M.D.2 - onComplete()

@sbrannen
Copy link
Author

@sbrannen There is a problem with the test

Oops. That's actually my fault: I missed that the bufferFactory was null at that point while migrating from JUnit 4 to JUnit Jupiter.

Thanks for finding that! I've fixed that now.

When we dispose the event loop group we invoke
netty.io/4.1/api/io/netty/util/concurrent/EventExecutorGroup.html#shutdownGracefully--
This default implementation is with 2s quiet period and 15s timeout

I've moved our "destroy" invocation to an @AfterAll method so that we are only hit with the 2s penalty once for the entire test class instead of for each test method.

This is acceptable and much better than the 122 seconds we were seeing.

However, is it possible to override the default quiet period and timeout?

At a glance, it looks like io.netty.util.concurrent.AbstractEventExecutor.DEFAULT_SHUTDOWN_QUIET_PERIOD is always used.

sbrannen added a commit to spring-projects/spring-framework that referenced this issue Sep 24, 2019
Prior to this commit, the parameterized DataBufferFactory was never
actually used when setting up the WebClient for each test. This was due
to an oversight when migrating from JUnit 4 to JUnit Jupiter.

See: reactor/reactor-netty#860

This commit fixes this by converting the existing @beforeeach method to
a local setup method that is invoked from each
@ParameterizedDataBufferAllocatingTest method.

In addition, in order to avoid the 2 second "quiet period" that is
incurred when destroying the ReactorResourceFactory, this commit moves
the setup and destruction of the ReactorResourceFactory to new
@BeforeAll and @afterall methods.

The test instance lifecycle has also been switched to PER_CLASS to avoid
static state in the test class.
@violetagg
Copy link
Member

@sbrannen
Copy link
Author

Closing since the bug in Spring's test suite has been fixed in spring-projects/spring-framework@38052e7.

@sbrannen
Copy link
Author

@sbrannen we can expose an API and then use https://netty.io/4.1/api/io/netty/util/concurrent/EventExecutorGroup.html#shutdownGracefully-long-long-java.util.concurrent.TimeUnit-
with the provided quiet period and timeout

I think it would be nice to make that configurable. Feel free to reopen this issue if you decide to go that route.

Thanks again for your support! 👍

@violetagg
Copy link
Member

Reopen - we may expose an API with quiet period and timeout

@violetagg violetagg reopened this Sep 24, 2019
@violetagg violetagg added type/enhancement A general enhancement and removed for/user-attention This issue needs user attention (feedback, rework, etc...) labels Sep 24, 2019
@violetagg violetagg added this to the 0.9.x Backlog milestone Sep 24, 2019
@violetagg violetagg added the good first issue Ideal for a new contributor, we'll help label Oct 30, 2019
@violetagg violetagg modified the milestones: 0.9.x Backlog, General Backlog Oct 30, 2019
@bilak
Copy link

bilak commented Dec 2, 2019

I'd like to have this feature in 0.9.x is it possible to provide it? We discussed it on gitter @violetagg .
Thanks

@violetagg violetagg modified the milestones: General Backlog, 0.9.x Maintenance Backlog Dec 2, 2019
@violetagg violetagg modified the milestones: 0.9.x Maintenance Backlog, 0.9.3.RELEASE Jan 8, 2020
violetagg added a commit that referenced this issue Jan 8, 2020
violetagg added a commit that referenced this issue Jan 9, 2020
@sbrannen
Copy link
Author

Thanks for adding this support, @violetagg!

Looking forward to updating our tests once we're on reactor-netty 0.9.3.

Do you know which reactor-bom release that will go into?

@violetagg
Copy link
Member

@sbrannen the planning is Dysprosium-SR3 for next Monday 13th of January

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Ideal for a new contributor, we'll help type/enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants