Handle Sighup #11

jimson-msft · 2021-03-15T20:38:30Z

No description provided.

jimson-msft · 2021-03-15T20:39:18Z

@shishirb-MSFT This is a reupload of the previous pr, unfortunately when we created the snapshot the PR did not get copied over, mind re-reviewing? #Closed

jimson-msft · 2021-03-15T20:39:32Z

client-lite/src/config/config_manager.cpp

+    _adminConfigs.Refresh();
+}
+
+


nit: space #Closed

shishirb-MSFT · 2021-03-15T20:46:42Z

client-lite/src/util/do_json_parser.cpp

+JsonParser::JsonParser(const std::string& jsonFilePath, bool alwaysCreateFile) :
    _jsonFilePath(jsonFilePath)
 {
+    if (!boost::filesystem::exists(_jsonFilePath) && alwaysCreateFile)


alwaysCreateFile [](start = 53, length = 16)

Check alwaysCreateFile first (cheaper) in the conditional. Same comment for the force check on line 28. #Closed

It's okay but curious why you chose to split into separate if statements rather than combine with &&.

In reply to: 594671740 [](ancestors = 594671740)

shishirb-MSFT · 2021-03-15T20:50:18Z

client-lite/src/download/download_manager.cpp


+void DownloadManager::RefreshConfigs() const
+{
+    _taskThread.SchedBlock([&]()


SchedBlock [](start = 16, length = 10)

No need to block, so use Sched() with 'this' as the tag. #Closed

Should be SchedImmediate() so downloads immediately get the new configs I think

In reply to: 594674119 [](ancestors = 594674119)

shishirb-MSFT · 2021-03-15T20:53:45Z

client-lite/src/exe/docs.cpp

            {
                break;
            }
+            if (_refreshEvent.IsSignaled())


_refreshEvent [](start = 16, length = 13)

Combining the event signaled check with the idle loop means we still have a delay in reading config values.
Instead, accept fnRefreshConfigs in the constructor and store it as a static member and call it from _SignalHandler directly. #Resolved

I did some additional refactoring that might make the class more flexible if we need to support other signals, but let me know what you think

In reply to: 594676272 [](ancestors = 594676272)

It's strange to have a callback function go back into the caller to achieve its task. It is more straightforward to have ProcessController itself handle the shutdown in this case.
Moreover, you now have two copies of the lambda to handle SIGINT and SIGTERM.

In general, refactoring early to support a future unknown requirement is not always good because you never know whether the change as it is today will apply to that requirement.

In reply to: 595355095 [](ancestors = 595355095,594676272)

True, alright will revert

In reply to: 595415616 [](ancestors = 595415616,595355095,594676272)

shishirb-MSFT · 2021-03-15T20:54:16Z

client-lite/src/exe/docs.cpp

            DoLogInfo("Received signal %d. Initiating shutdown.", signalNumber);
            _shutdownEvent.SetEvent();
        }
+        if (signalNumber == SIGHUP)


if [](start = 8, length = 2)

else if #Closed

shishirb-MSFT · 2021-03-15T20:55:23Z

client-lite/src/download/download_manager.h

    DownloadStatus GetDownloadStatus(const std::string& downloadId) const;

    bool IsIdle() const;
+    void RefreshConfigs() const;


RefreshConfigs [](start = 9, length = 14)

RefreshAdminConfigs - to be clear and consistent. #Closed

shishirb-MSFT · 2021-03-15T20:55:40Z

client-lite/src/config/mcc_manager.h

 #include <unordered_map>
 #include "ban_list.h"
-
-class ConfigManager;


ConfigManager [](start = 6, length = 13)

fwd declaration should be enough here #Closed

…n processcontroller

jimson-msft · 2021-03-16T16:45:11Z

client-lite/src/exe/docs.cpp

 using namespace std::chrono_literals; // NOLINT(build/namespaces) how else should we use chrono literals?

-class ProcessController
+class ProcessController : DONonCopyable


: DONonCopyable [](start = 24, length = 15)

Perhaps there is no need to make this a singleton as there are no non-static members #Resolved

Non-copyable doesn't make it a singleton because you can create multiple instances of this class.

In reply to: 595354139 [](ancestors = 595354139)

Ah right, there's a convention for singleton involving static instance of the class itself and a private constructor

In reply to: 595411879 [](ancestors = 595411879,595354139)

shishirb-MSFT · 2021-03-16T18:02:56Z

client-lite/src/download/download_manager.cpp


+void DownloadManager::RefreshAdminConfigs() const
+{
+    _taskThread.SchedImmediate([&]()


& [](start = 32, length = 1)

Capture 'this' instead. The capture-all option is generally discouraged because it will capture more stuff than is needed. There's nothing other than 'this' to capture here but better to get used to that practice. #Closed

shishirb-MSFT · 2021-03-16T18:04:59Z

client-lite/src/exe/docs.cpp

+        else if (signalNumber == SIGHUP)
+        {
+            _sighupHandler = std::move(fnHandler);
+        }


} [](start = 8, length = 1)

Depending on how this will change based on my refactoring comment, add an else case here with DO_ASSERT(false). #Resolved

Think I'll revert the changes and just make the constructor accept the callback for refreshing configs

In reply to: 595419554 [](ancestors = 595419554)

shishirb-MSFT · 2021-03-16T22:07:34Z

client-lite/src/exe/docs.cpp

 {
 public:
-    ProcessController()
+    ProcessController(std::function<void()>& fnRefreshConfigs)


& [](start = 43, length = 1)

Since you are using std::move, use && here #Closed

shishirb-MSFT · 2021-03-16T22:10:17Z

client-lite/src/exe/docs.cpp

    DoLogInfo("Started, %s", msdoutil::ComponentVersion().c_str());

-    ProcessController procController;
+    std::function<void()> fnRefreshConfigs = [&downloadManager]()


std::function<void()> [](start = 0, length = 25)

Can use auto here but consider writing the lambda inline on line 111. It will also satisfy the && parameter type. #Closed

Right, able to write inline due to parameter type being an rvalue reference with the change to use && above

In reply to: 595575443 [](ancestors = 595575443)

jimson-msft · 2021-03-16T22:27:54Z

Verified this change works on iteration 5.
Modified /etc/deliveryoptimization/admin-config.json to contain:
{
"DOCacheHost": "www.microsoft.com"
}
ran sudo systemctl kill -s SIGHUP deliveryoptimization-agent.service to refresh configs

With do plugin installed, did an apt install of npm and verified through logging that client configs were refreshed, bad mcc host was banned, and download finished succesfully. #Closed

shishirb-MSFT

initial commit of handle sighup code

5b4c5ef

jimson-msft requested a review from a team as a code owner March 15, 2021 20:38

jimson-msft commented Mar 15, 2021

View reviewed changes

client-lite/src/config/config_manager.cpp Outdated

_adminConfigs.Refresh();

}

Copy link

Contributor Author

jimson-msft Mar 15, 2021 •

edited

Loading

Choose a reason for hiding this comment

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

nit: space #Closed

shishirb-MSFT reviewed Mar 15, 2021

View reviewed changes

jimson-msft added 2 commits March 15, 2021 19:13

cleanup pr, modify signal handling to not use the polling mechanism i…

3e30702

…n processcontroller

refactor proc controller

097ab81

jimson-msft commented Mar 16, 2021

View reviewed changes

shishirb-MSFT reviewed Mar 16, 2021

View reviewed changes

jimson-msft added 2 commits March 16, 2021 14:49

revert changes to proc controller

d6939c9

revert noncopyable change

25bfe4a

shishirb-MSFT reviewed Mar 16, 2021

View reviewed changes

resolve pr comments

5f67d33

shishirb-MSFT approved these changes Mar 16, 2021

View reviewed changes

jimson-msft merged commit 713fe55 into main Mar 16, 2021

jimson-msft deleted the user/jimson/handle_sighup branch March 16, 2021 23:05

Handle Sighup #11

Handle Sighup #11

Uh oh!

Conversation

jimson-msft commented Mar 15, 2021

Uh oh!

jimson-msft commented Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimson-msft Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021 • edited by shishirb-MSFT Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimson-msft Mar 16, 2021

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

jimson-msft commented Mar 15, 2021 •

edited

Loading

jimson-msft Mar 15, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Mar 16, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited

Loading

shishirb-MSFT Mar 15, 2021 •

edited

Loading

jimson-msft Mar 16, 2021 •

edited by shishirb-MSFT

Loading

shishirb-MSFT Mar 16, 2021 •

edited

Loading

shishirb-MSFT Mar 16, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Mar 16, 2021 •

edited

Loading

shishirb-MSFT Mar 16, 2021 •

edited

Loading

jimson-msft Mar 16, 2021 •

edited

Loading

jimson-msft commented Mar 16, 2021 •

edited

Loading