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

Update to file-leak-detector:1.11 to detect pipe and selector leaks #3

Merged
merged 14 commits into from
Apr 16, 2018

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Apr 6, 2018

Subsumes #2.

Updates the parent pom and specifies explicit Jenkins and Java versions, pulls in jenkinsci/lib-file-leak-detector#34 which adds support for detecting Pipes, Selectors, and files opened via FileChannel, and adds some basic tests.

@reviewbybees

Copy link
Contributor

@denisj44 denisj44 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the Idea file coming from the editor should be removed.

.gitignore Outdated
@@ -1,2 +1,4 @@
target
work
.idea
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think idea editor specific files needs to be added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you added it originally in 504dc08 😄

I will go ahead and remove it, but for what it's worth .idea and other IntelliJ/Eclipse files are added to .gitignore by default for any plugins using the maven archetype generator: https://github.com/jenkinsci/archetypes/blob/master/empty-plugin/src/main/resources/archetype-resources/.gitignore. We could follow up with a separate PR to add all of those entries to .gitignore if desired.

@jglick jglick mentioned this pull request Apr 11, 2018
*/
private void activateFileLeakDetector() throws Exception {
FileHandleDump fileHandleDump = ManagementLink.all().get(FileHandleDump.class);
fileHandleDump.doActivate("");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fails on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I'll investigate to see whether we can fix it here or if the library itself doesn't work on Windows.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really have no idea. You can always

buildPlugin(platforms: ['linux'])

if it is known not to work on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to JNR fixed the main issue, but pipes aren't detected correctly on Windows by the library, so I've ignored that test on Windows for now. See jenkinsci/lib-file-leak-detector#36.

@dwnusbaum dwnusbaum merged commit 4ad9e27 into jenkinsci:master Apr 16, 2018
@dwnusbaum dwnusbaum deleted the file-leak-detector-1.11 branch April 16, 2018 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants