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

Add Selector support #34

Merged
merged 5 commits into from
Mar 27, 2018
Merged

Add Selector support #34

merged 5 commits into from
Mar 27, 2018

Conversation

dwnusbaum
Copy link
Member

@dwnusbaum dwnusbaum commented Jan 18, 2018

Based on top of #33 to pick up useful pom changes and validate against the extra tests. I am happy to submit a PR against master with only my commits if desired.

This is not going to be completely accurate, as what file descriptors are opened by a selector vary from platform to platform, but is simpler than transforming all of the different native classes to track each descriptor separately. For example, EPollSelectorImpl on Linux will open 2 pipes and an anonymous inode for event polling, but this PR only tracks it as a single descriptor.

This seems ok to me, since those file descriptors will be opened and closed together anyway (barring any JDK bugs), and once you know that you are leaking Selectors you can figure out exactly what file descriptors are being created on your platform for each Selector.

@reviewbybees

@ghost
Copy link

ghost commented Jan 18, 2018

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev
Copy link
Member

Cannot assign myself as a reviewer, but I will try to review it next week

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

I have tested it locally and it works well. I'd guess minor Binary compatibility changes are not a problem for this Lib/CLI, hence 🐝

I also tested it with jenkinsci/file-leak-detector-plugin#2 from @denisj44. It seems to work well though I had some issues with building the plugin on Java 8

@dwnusbaum
Copy link
Member Author

@reviewbybees done

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.

4 participants