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

wait for network after resuming from suspend #11277

Merged
merged 1 commit into from Dec 31, 2016

Conversation

@verybadsoldier
Copy link
Contributor

commented Dec 27, 2016

Makes Kodi wait for network connection after resuming from suspend.

Description

This introduces a function WaitForNet in Kodi's power manager which will block (with a timeout) until at least one network interface is initialized and connected. This function is invoked in OnWake so it is triggered on every wake up.
This behavior is disabled by default and has to be activated by a new advanced setting:

    <suspend>
        <waitnetonwakeup>true</waitnetonwakeup>
    </suspend>

This new setting would have to be documented on the Wiki I guess.

Motivation and Context

When waking the machine from sleep/suspend and directly trying to access network resources (e.g. playing a video or reading from the MySQL DB) then Kodi will fail cause the network has not been initialized yet.
I have experienced and described this issue here:
http://forum.kodi.tv/showthread.php?tid=301976

How Has This Been Tested?

I tested it on a Intel NUC i5 using LibreELEC 7.02 using a wired network. I tested it by suspending the machine and waking it up again. Then I observed the behavior with and without this setting enabled. With the setting enabled Kodi blocked for several seconds and put something like this in the logfile:

12:51:10 T:140441393608768  NOTICE: OnWake: Running resume jobs
12:51:10 T:140441393608768  NOTICE: WaitForNet: Waiting for first network interface to come up
12:51:18 T:140441393608768  NOTICE: WaitForNet: network interface 'eth0' is up after waiting 7400 ms
12:51:18 T:140441393608768  NOTICE: OnWake: Restarting lirc

After this I could use Kodi and directly access network resources without issues.

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the Code guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the CONTRIBUTING document
  • I have added tests to cover my change
  • All new and existing tests passed
@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 27, 2016

no advanced settings. those are only valuable for the very few who know about them. mostly only the developer who implemented it.

@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2016

@FernetMenta
Okay, so it should be a regular setting configurable in the UI? I will try to find out how to do this.

So the general concept is accetable and might get merged if the setting is changed?


for(unsigned i=0; i < maxLoopCount; ++i)
{
CNetworkInterface* pIface = g_application.getNetwork().GetFirstConnectedInterface();

This comment has been minimized.

Copy link
@FrankHeimes

FrankHeimes Dec 27, 2016

Consider replacing CNetworkInterface* with const auto * const if GetName() is const or const auto otherwise.

@@ -93,6 +93,8 @@ class CPowerManager : public IPowerEventsCallback

void OnLowBattery();

void WaitForNet();

This comment has been minimized.

Copy link
@FrankHeimes

FrankHeimes Dec 27, 2016

Consider declaring WaitForNet() const.

@@ -383,6 +383,8 @@ class CAdvancedSettings : public ISettingCallback, public ISettingsHandler

std::string m_userAgent;

bool m_waitForNetAfterWakeUp;

This comment has been minimized.

Copy link
@FrankHeimes

FrankHeimes Dec 27, 2016

Consider initializing member variable when declaring it: bool m_waitForNetAfterWakeUp = true;

if (pElement)
{
XMLUtils::GetBoolean(pElement, "waitnetonwakeup", m_waitForNetAfterWakeUp);
}

This comment has been minimized.

Copy link
@FrankHeimes

FrankHeimes Dec 27, 2016

CAdvancedSettings::ParseSettingsFile() is ridiculously long and should be refactored into local free functions.
That is too much work for a single feature implementation, but not adding more inline code would be a start.

Consider putting lines 1202 - 1206 into a free function

namespace {
	bool GetWaitNetOnWakeupFlag(TiXmlElement *rootElement)
	{
		if (!rootElement)
			throw std::invalid_argument("rootElement");

		auto firstChild = rootElement->FirstChildElement("suspend");
		if (!firstChild)
			return true; // Default behavior is to enable waitnetonwakeup

		bool waitForNetAfterWakeUp = true;
		XMLUtils::GetBoolean(firstChild, "waitnetonwakeup", waitForNetAfterWakeUp);
		return waitForNetAfterWakeUp;
	}
}

The line 1202: m_waitForNetAfterWakeUp = GetWaitNetOnWakeupFlag(); makes clear that the member is always well defined. Now, this is a lot more code than the original, but it is self-contained. pRootElement is checked for NULL, but this happens in line 444, i.e. 750+ lines above. Any (future changes) in between could invalidate it.

BTW.: IMHO, XMLUtils should provide function-style Getters, too, that don't require handing over a reference variable.

Yes, I'm new to this group and I do the code review like I do at work. Am I pushing too hard?

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Dec 27, 2016

Member

We welcome constructive reviews 👍

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Dec 30, 2016

Member

I thought you were a bot at first since the comments were so methodical and clean. +1

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 27, 2016

@verybadsoldier for what reason should we wait not always for network to be ready?

@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2016

@FernetMenta
For me it would be fine if it was just hardwired but I am afraid it could be a problem for systems without network (e.g. no NIC at all or no cable connected). Also there might be users who run Kodi exclusively from local storage and do not care for network connections.
In both cases an unwanted delay would be introduced on wakeup. In my case it takes about 7 seconds for the network to beccome available.

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch 3 times, most recently from 607e2af to cfdd488 Dec 28, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2016

Reworked so that this only kicks in if there is at least one network interface. This will prevent waiting for network when there are no network Interfaces in the system. So maybe I really could just remove the advanced setting and make waiting for network after wakeup the standard behavior.

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 28, 2016

Regardless an advancedsettings is out of the question here

@t4-ravenbird

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2016

The original problem that this PR seeks to solve is already possible to fix by enabling the built-in wake-on-lan feature.

That solution works the same when starting up, and when resuming from suspend. There have been MySQL issues in both cases and they are both solved when using wake-on-lan, for instance ;
http://forum.kodi.tv/showthread.php?tid=213321&pid=1879249#pid1879249
http://forum.kodi.tv/showthread.php?tid=173267&pid=1820808#pid1820808

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from cfdd488 to cec30ba Dec 28, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2016

Okay, removed the advanced settings.

@t4-ravenbird
Yes, the wake-on-lan feature can help here but to be honest I see it only as a workaround:

  • the original problem is not related to WOL, so having to enable the WOL-feature feels a bit "hackish"
  • the WOL-wait does not really wait until the network connection is established, according to the code it seems to just wait the configured period of time regardless of the network state
  • it only kicks in when the network is actually accessed (which makes sense in the WOL context), but in my opinion Kodi's network itself should be up and working after resuming
@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from cec30ba to 8d4b6e5 Dec 28, 2016
@t4-ravenbird

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2016

WOL-wait waits until the correct network interface is available before proceeding (As opposed to just waiting for 1st interface)
Any thread accessing the network resource is held back until it is ready - any other threads are free to run.

@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 29, 2016

I fiddled around lots with this WOL-wait today and it does not really fix the problem. In some cases it did help but in most cases it did not get triggered when accessing network resources after resuming.


CLog::Log(LOGNOTICE, "%s: Waiting for a network interface to come up", __FUNCTION__);

const static int numMaxTries = 50;

This comment has been minimized.

Copy link
@FernetMenta

FernetMenta Dec 30, 2016

Member

those are kind of ugly. I would make them configurable as max waiting time. a value of zero disables this feature.

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch 5 times, most recently from b514be2 to ff874b5 Dec 30, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Agreed, I changed the loop and also added a setting which is configurable in the UI under "System -> Power saving".

image

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

I think a max of 120 seconds is a bit much. By that time i would have throw it out the window. 30 might be more realistic although still a lot

@stefansaraev

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2016

there is an unwanted file mode (0644 -> 0755) change in this PR. please chmod everything back to 0644

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from ff874b5 to f66165f Dec 30, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 30, 2016

Reduced maximum timeout value to 30.

Thanks for pointing out the mode changes, fixed. Sorry for that.

@FernetMenta

This comment has been minimized.

Copy link
Member

commented Dec 30, 2016

looks good to me
thanks!

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

See my comments about the text. Once those are done we can let it build and merge it in.


#: system/settings/settings.xml
msgctxt "#39011"
msgid "Wait for network after waking up"

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Dec 31, 2016

Member

Please make it "Maximum wait time for network" as this directly indicates what it does without reading the help text.

#. Description of setting with label #39011 "Wait for network after waking up"
#: system/settings/settings.xml
msgctxt "#39012"
msgid "Set the maximum time to wait for the network to come up after waking up."

This comment has been minimized.

Copy link
@MartijnKaijser

MartijnKaijser Dec 31, 2016

Member

"Set the maximum time to wait for the network to come up after starting or waking up. When time has passed before network is up startup will continue."
If it indeed also handles a startup (which it should imo)

@chewitt

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

@verybadsoldier Does this also handle "wait on startup" ?? if not, could it do both? There's been a long-standing issue for MySQL users where Kodi starts and the network stack isn't ready. LE has a delayed start workaround function that works, but it would be cleaner if Kodi was able to handle it internally.

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from f66165f to 4d9eea2 Dec 31, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2016

Ok, I have changed the wording, thanks for the feedback.

Currently it does only handle the wait when waking up. I will take a look to reuse this feature also on startup. But if possible I would like to finish this first and then do it in a separate PR if that would be ok for. I promise to come back :)

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

👍 might want to remove "starting or" part of the strin then and we're ready to go

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from 4d9eea2 to e11dce6 Dec 31, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2016

Oh, good point, fixed :D Thanks!

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

jenkins build this please

@MartijnKaijser MartijnKaijser added this to the L 18.0-alpha1 milestone Dec 31, 2016
@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

Jenkins build and merge

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

Jenkins build this please

@verybadsoldier verybadsoldier force-pushed the verybadsoldier:net_after_suspend branch from e11dce6 to 958469a Dec 31, 2016
@verybadsoldier

This comment has been minimized.

Copy link
Contributor Author

commented Dec 31, 2016

(Hopefully) fixed header issue for Android/iOS. Would you please trigger Jenkins again to see if it works?
Sorry I cannot test this on my end :(

@MartijnKaijser

This comment has been minimized.

Copy link
Member

commented Dec 31, 2016

No problem. That why we have jenkins build this please

@MartijnKaijser MartijnKaijser merged commit 112fc81 into xbmc:master Dec 31, 2016
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
jenkins4kodi You did a great job. Have a cookie.
Details
@verybadsoldier verybadsoldier referenced this pull request Jan 1, 2017
2 of 8 tasks complete
@verybadsoldier verybadsoldier deleted the verybadsoldier:net_after_suspend branch Jan 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.