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

[WFCORE-774] Memory leak in JBoss CLI #866

Merged
merged 1 commit into from Jul 8, 2015

Conversation

TomasHofman
Copy link
Contributor

@wildfly-ci
Copy link

Core - Full Integration Build 1264 is now running using a merge of f2140f9

@wildfly-ci
Copy link

Linux Build 2130 is now running using a merge of f2140f9

@wildfly-ci
Copy link

Windows Build 1663 is now running using a merge of f2140f9

@wildfly-ci
Copy link

Windows Build 1663 outcome was SUCCESS using a merge of f2140f9
Summary: Tests passed: 3364, ignored: 63 Build time: 0:33:15

@wildfly-ci
Copy link

Linux Build 2130 outcome was SUCCESS using a merge of f2140f9
Summary: Tests passed: 3364, ignored: 63 Build time: 0:34:47

@wildfly-ci
Copy link

Core - Full Integration Build 1264 outcome was SUCCESS using a merge of f2140f9
Summary: Tests passed: 2906, ignored: 372, muted: 1 Build time: 0:47:33

@aloubyansky
Copy link
Contributor

There is a method in CommandContext called terminateSession() which is where the clean up should take place when the session is over. Is there anything that could also be added to that method?

@TomasHofman
Copy link
Contributor Author

Well, the only problem there was that every instance of CommandContextImpl added new JaasConfigurationWrapper to global Configuration (so it was creating longer and longer chain). JaasConfigurationWrappers are also holding references back to CommandContextImpl instances, which prevents those from being garbage collected.

So I see two options:

  1. only add JaasConfigurationWrapper to global Configuration in the first CommandContext instance being created (shouldn't affect the functionality as I believe),
  2. or remove JaasConfigurationWrapper from the Configuration in terminateSession() and restore original configuration.

Neither option is bullet proof, and I thought 1) is simpler.

@aloubyansky
Copy link
Contributor

@darranl what's your opinion on this?

@aloubyansky
Copy link
Contributor

BTW, Tomas, thanks for your effort here!

@darranl
Copy link
Contributor

darranl commented Jul 8, 2015

Looks fine.

@aloubyansky
Copy link
Contributor

Ok, given that no matter which option we choose it's still a static config at the end, I'm fine with this too.

@bstansberry bstansberry added the ready-for-merge This PR is ready to be merged and fulfills all requirements label Jul 8, 2015
bstansberry added a commit that referenced this pull request Jul 8, 2015
[WFCORE-774] Memory leak in JBoss CLI
@bstansberry bstansberry merged commit 669eef7 into wildfly:master Jul 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to be merged and fulfills all requirements
Projects
None yet
5 participants