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

Projects
None yet
4 participants
@nextgens
Copy link
Contributor

nextgens commented Dec 17, 2015

This is something I've introduced in 7487c51

@nextgens

This comment has been minimized.

Copy link
Contributor

nextgens commented Dec 17, 2015

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

@toad

This comment has been minimized.

Copy link
Member

toad commented Dec 17, 2015

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

@toad

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

nextgens commented Dec 19, 2015

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

This comment has been minimized.

Copy link
Member

Thynix commented Dec 19, 2015

Oh. Alright.

@bertm

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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 freenet:next Dec 19, 2015

@nextgens nextgens deleted the nextgens:bug6623 branch Jan 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment