-
Notifications
You must be signed in to change notification settings - Fork 912
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
ARTEMIS-2408 Too many opened FDs after server stops #2749
Conversation
de1dbfe
to
63aa1c3
Compare
/** | ||
* Utility adapted from: org.apache.activemq.util.Wait | ||
*/ | ||
public class Wait { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we avoid duplicating this, and simply make it more shareable between modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@brusdev you did this on top of my previous commit, I had the Wait there as a hack, and I intended to fix this before merging.
We need to fix the Wait here.
(I think you should have kept my commit as a reference of my ownership though and changed it on top.. but no big deal)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a separate issue. It's about the artemis-junit project duplicating some stuff.
I'm removing the dependency on artemis-junit, and I'm dealing with this on a separate PR. I am merging this and i will send a new PR.
@@ -57,6 +57,11 @@ | |||
import org.apache.activemq.artemis.spi.core.remoting.Acceptor; | |||
import org.apache.activemq.artemis.spi.core.security.ActiveMQJAASSecurityManager; | |||
import org.apache.activemq.artemis.tests.util.ActiveMQTestBase; | |||
import org.fusesource.hawtdispatch.DispatchPriority; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fusesource actually maintained, i see last update 2016, do we really want to be building further on it???
Can we implement using alternative lib or our own for better maintenance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FuseMQTTClientProvider (already used by some MQTT tests) has a transitive dependency for org.fusesource.hawtdispatch: HawtDispatcher is statically allocated by DispatcherConfig when org.fusesource.mqtt.client.MQTT.blockingConnection is called and It create a new GlobalDispatchQueue.
I used org.fusesource.hawtdispatch only to shuthdown the global queue at the end of the test to avoid file descriptor leaks.
/** | ||
* This is useful to make sure you won't have leaking threads between tests | ||
*/ | ||
public class NoFDBehind extends TestWatcher { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats FD?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed NoFDBehind to NoProcessFilesBehind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FileDescriptors
@@ -171,6 +171,9 @@ | |||
@ClassRule | |||
public static ThreadLeakCheckRule leakCheckRule = new ThreadLeakCheckRule(); | |||
|
|||
@Rule | |||
public NoFDBehind noFDBehind = new NoFDBehind(-1, 1000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whats FD, i can guess due to context of this PR, but the point is, i shouldnt have to guess and in future without PR context it will be harder to guess, please avoid using acronym or shorthand, for others readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed noFDBehind to noProcessFilesBehind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaelandrepearce I would have thought that FD is a common usage for File Descriptors?
@brusdev i would also appreciate if we could finish off the changes in https://github.com/apache/activemq-artemis/pull/2727/files, worried thats taken a back seat, and is on the back of an already merged PR change |
f1dc305
to
dc5600a
Compare
Wait netty event loop group shutdown to avoid too many opened FDs after server stops, when netty configuration is used. Clear server activateCallbacks to avoid reactivation of previous nodeManager and consequent FD leaks on restart. Fix LargeServerMessageImpl.copy to avoid FD leaks when a large message expiry or it is sent to DLA. Terminate HawtDispatcher global queue to avoid pipes and eventpolls leaks after a MQTT test. cherry-picking commit 9617058 NO-JIRA adding check for open FD on the testsuite cherry-picking commit 0facb7d NO-JIRA addressing connections leaks on integration tests
dc5600a
to
bcb10ce
Compare
Wait netty event loop group shutdown to avoid too many opened FDs after
server stops, when netty configuration is used. Clear server
activateCallbacks to avoid reactivation of previous nodeManager and
consequent FD leaks on restart. Fix LargeServerMessageImpl.copy to avoid
FD leaks.
cherry-picking commit 9617058
NO-JIRA adding check for open FD on the testsuite
cherry-picking commit 0facb7d
NO-JIRA addressing connections leaks on integration tests