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

Fix bug #6623 : Wrapper.log: Lots of "setNativePriority(X) has failed!" #435

Merged
merged 3 commits into from Dec 19, 2015

Conversation

nextgens
Copy link
Contributor

This is something I've introduced in 7487c51

@nextgens
Copy link
Contributor Author

@toad thinks it's a must-have for next release

@toad
Copy link
Contributor

toad commented Dec 17, 2015

Yes, but we should fix the API too before closing #6623.

@toad
Copy link
Contributor

toad commented Dec 17, 2015

This is the correct fix for build 1471, i.e. a one line fix and javadocs. I have filed a bug for a longer term solution.

@Thynix
Copy link
Contributor

Thynix commented Dec 19, 2015

Bug in question is https://bugs.freenetproject.org/view.php?id=6743

This fix is the same as the patch in https://bugs.freenetproject.org/view.php?id=6623 and as bertm says

This patch does not address the underlying problem causing the messages. It merely disables priority checking of the new thread, which would fail otherwise, because apparently we are creating this NativeThread from a low-priority NativeThread (that appears as having been reniced).

I'd rather not merge a half-measure like this that just disables the logging.

@nextgens
Copy link
Contributor Author

Respectfully, @bertm is wrong :)

This is not a half-measure, it's the real fix. I'm the one who introduced the said buggy code and we've extensively discussed it with toad already.

The check behind the boolean doesn't make much sense (and yes it's also my code) and it definitely doesn't in this specific instance.

@Thynix
Copy link
Contributor

Thynix commented Dec 19, 2015

Oh. Alright.

@bertm
Copy link
Contributor

bertm commented Dec 19, 2015

You all seem to be reading my comment out of context. This does not fix the logging behaviour bug 6623 reported, but does fix this particular source of the error (and adds Javadoc to hopefully prevent future instances). If NativeThreads are somehow disabled—i.e. Fred is reniced, which it shouldn't be, but heh—it will still spit out hundreds of setNativePriority(…) has failed! lines. However: that is only a logging bug that this pull request does not seem to intend to, nor has to, fix.

I do however agree that this fixes this particular instance where NativeThreads are disabled inappropriately, and give my full ack for merging.

@Thynix
Copy link
Contributor

Thynix commented Dec 19, 2015

Whoops, I was not intending to quote you out of context. Sorry about that.

@Thynix Thynix merged commit 8723e72 into hyphanet:next Dec 19, 2015
@nextgens nextgens deleted the bug6623 branch January 2, 2016 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants