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

[Network] fix oom bug #4827

Merged
merged 5 commits into from
Feb 11, 2019
Merged

[Network] fix oom bug #4827

merged 5 commits into from
Feb 11, 2019

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Feb 7, 2019

Fixes #3145

Large networks (like /8) caused an OOM exception because 16 million IP addresses were cconstructed during network discovery. This reduces the network size to the number of requested addresses (usually 255 which results in /24).

Verified locally.

@J-N-K J-N-K requested a review from davidgraeff as a code owner February 7, 2019 17:18
@davidgraeff
Copy link
Member

Hm. Why did that happen in the first place. maximumPerInterface should have limited the number of IP addresses per interface.

@J-N-K
Copy link
Member Author

J-N-K commented Feb 7, 2019

Old l. 139 constructs an array of all addresses which is later truncated to the number of requested addresses in l. 144.

This fails on systems with low memory (on my development machine I wasn‘t able to reproduce it until I reduced the vm heap size).

@davidgraeff
Copy link
Member

Ah ok. The problem is the SubnetUtils class, which does not provide means to limit the returned addresses. So this workaround is to modify the subnet, I see.

But there is also an IPv4 matcher included. That should be removed, because the interface IPs are provided by getInterfaceIPs which should only return IPv4 addresses. (I checked the code and it does not so atm, the correct fix is to check if the address is an instance of IPv4Address in

https://github.com/openhab/openhab2-addons/blob/fe8cbb0e79707064049a7984f2e41e84b4e53730/addons/binding/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java#L91

@J-N-K
Copy link
Member Author

J-N-K commented Feb 7, 2019

I can add that there, too. But I need the matcher anyway to separate address and CIDR suffix, so it will result in a double check.

@davidgraeff
Copy link
Member

davidgraeff commented Feb 7, 2019

Took some time but I have found it:

https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/net/CidrAddress.java

There is a class in the core bundle (also in eclipse smarthome core) that wraps an InetAddress and a prefix length. We can just return that in getInterfaceIPs and the prefixLength check can be much shorter in the method in question.

See also: https://github.com/openhab/openhab-core/blob/master/bundles/org.openhab.core/src/main/java/org/eclipse/smarthome/core/net/NetUtil.java#L373

That is a static method that makes the network binding method superfluous.

@J-N-K
Copy link
Member Author

J-N-K commented Feb 7, 2019

I‘ll have a look.

Signed-off-by: Jan N. Klug <[email protected]>
@J-N-K
Copy link
Member Author

J-N-K commented Feb 10, 2019

@davidgraeff, I also removed the exception handling whcih was only necessary if an invalid address was passed to new SubnetUtils(address).

Signed-off-by: Jan N. Klug <[email protected]>
Copy link
Member

@davidgraeff davidgraeff left a comment

Choose a reason for hiding this comment

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

LGTM

@davidgraeff davidgraeff merged commit 0a5e0fc into openhab:master Feb 11, 2019
@J-N-K J-N-K deleted the network-fix-oom branch February 13, 2019 18:12
@wborn wborn added this to the 2.5 milestone Feb 28, 2019
jannegpriv pushed a commit to jannegpriv/openhab-addons that referenced this pull request Mar 3, 2019
fix oom bug (openhab#4827)

Signed-off-by: Jan N. Klug <[email protected]>
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/network-binding-problem-heap-space/72243/2

Pshatsillo pushed a commit to Pshatsillo/openhab-addons that referenced this pull request Jun 19, 2019
fix oom bug (openhab#4827)

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Pshatsillo <[email protected]>
ne0h pushed a commit to ne0h/openhab-addons that referenced this pull request Sep 15, 2019
fix oom bug (openhab#4827)

Signed-off-by: Jan N. Klug <[email protected]>
Signed-off-by: Maximilian Hess <[email protected]>
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