Skip to content

Removing reflection use in NetUtils#2426

Open
lislei wants to merge 6 commits intoBiglySoftware:masterfrom
lislei:no_reflection
Open

Removing reflection use in NetUtils#2426
lislei wants to merge 6 commits intoBiglySoftware:masterfrom
lislei:no_reflection

Conversation

@lislei
Copy link
Contributor

@lislei lislei commented Jan 22, 2022

Removed forward compatibility mode using reflection. This is safe as the Android SDK level 15 / JDK8 API is the current baseline.
Wrapped the main error paths for network interface lookup into a separate methods and giving it functional names.

NetworkInterface.getByIndex(int) was introduced with Java 7.

collectNetworkInterfacesByIndex(list);

if (!list.isEmpty()) {
return Collections.enumeration(list);
Copy link
Contributor Author

@lislei lislei Jan 22, 2022

Choose a reason for hiding this comment

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

This "Java 7 preview" block of code made the runtime return at this line when running Java 7 or higher.

I believe the next block was for JDK 6 and earlier only, making that a tautology on the current baseline.
Should I go ahead and remove it too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, it only returned here if the list was non-empty. Which doesn't seem like it's guaranteed even still, so I'd say it's far from tautological.

Most of the time the code will never get this far, period. It'll return the valid list produced by getNetworkInterfaces() on line 526 (524 in your PR), none of the code past that will even execute, and all will be right with the world.

But on some system where getNetworkInterfaces() fails, can we say for sure that getByIndex() won't fail as well? Especially when the code is just guessing indices.

It's clear from the docs that the getByIndex() method was meant to be used by code that already had a known valid index, most likely retrieved by previously calling .getIndex() on an interface object and storing it somewhere. And the docs explicitly refuse to guarantee anything about the index values themselves, other than that they're integers >= 0. So there's nothing stopping some implementation from using high and/or randomized index values that the getByIndex() approach won't ever find.

In which case, guessing names could theoretically still work as an absolute last-resort fallback.

Copy link
Contributor

@ferdnyc ferdnyc Feb 13, 2023

Choose a reason for hiding this comment

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

I was right the first time, with what I said above. Ignore any followups, nothing to see here. 🙄 (Not my night.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for assessing this question. Your initial comment makes good sense.

I now believe this (and the next) error path are "best efforts" of reacting to runtimes throwing you exceptions from the underlying OS. They both give no guarantees of finding any interfaces what so ever. In the worst case - the fall trough - is simply re-throwing the initial exception.

I'll add a brief documentation describing the two error paths.

@lislei lislei force-pushed the no_reflection branch 2 times, most recently from 230f0d7 to 6206c65 Compare September 30, 2025 20:56
Using the documented tip from .editorconfig:
IntelliJ IDEA has Settings->Editor->General->Strip trailing spaces on Save that can be set to "Modified Lines" which sometimes works properly
@lislei lislei requested a review from ferdnyc September 30, 2025 21:11
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.

2 participants