Error Code Interface & Partially Remove Internal Exceptions #88

jimson-msft · 2021-10-02T20:31:22Z

Provide error code returning interface
Strip internal exceptions for win10
For rest, internal classes may call 3p libraries which have throwing methods, wrap these in try-catch. Will remove exceptions on REST side at a later point in time.
Add a error macro header, eventually will move into shared header.

shishirb-MSFT · 2021-10-06T12:03:18Z

CMakeLists.txt

 option (DO_INCLUDE_SDK "Build subproject sdk-cpp" OFF)

 option (DO_BUILD_TESTS "Set DO_BUILD_TESTS to OFF to skip building tests." ON)
+option (DO_ENABLE_EXCEPTIONS "Set DO_ENABLE_EXCEPTIONS to ON to disable compiling with exceptions" OFF)


DO_ENABLE_EXCEPTIONS

Since this applies to the sdk only, how about moving all changes in this file to the sdk cmakelists.txt? #Resolved

I see this is copied to sdk-cpp/cmakelists.txt. Delete from here?

shishirb-MSFT · 2021-10-06T12:05:01Z

CMakeLists.txt

 option (DO_INCLUDE_SDK "Build subproject sdk-cpp" OFF)

 option (DO_BUILD_TESTS "Set DO_BUILD_TESTS to OFF to skip building tests." ON)
+option (DO_ENABLE_EXCEPTIONS "Set DO_ENABLE_EXCEPTIONS to ON to disable compiling with exceptions" OFF)


OFF

I would default this to ON and let Edge disable it. #Closed

They probably don't need to do anything since they don't use cmake and by default DO_ENABLE_EXCEPTIONS will not be defined in their build system. Win-win.

nice, yea +1 on this

shishirb-MSFT · 2021-10-06T12:34:01Z

sdk-cpp/include/do_download.h

    */
    void set_property(download_property key, const download_property_value& value);
-    download_property_value get_property(download_property key);
+    void get_property(download_property key, download_property_value& value);


void

This should return the value instead, no? #Closed

Yeah I'll have it as a return val instead of an out param here

shishirb-MSFT · 2021-10-06T12:35:36Z

sdk-cpp/include/do_download_property.h

    ~download_property_value() = default;

+#if (DO_ENABLE_EXCEPTIONS)
+    static void make(download_property_value& out, const std::string& val);


download_property_value

Return this instead in all the throwing make methods. #Closed

shishirb-MSFT · 2021-10-06T12:36:20Z

sdk-cpp/include/do_download_property.h

+    void as(status_callback_t& val) const;
+#endif
+
+    static int32_t make_nothrow(download_property_value& out, const std::string& val);


out

Out params come after the In. #Closed

shishirb-MSFT · 2021-10-06T12:37:00Z

sdk-cpp/include/do_download_status.h

+
+} // namespace deliveryoptimization
+} // namespace microsoft
+#endif


// _DELIVERY_OPTIMIZATION_DO_DOWNLOAD_STATUS_H #Closed

shishirb-MSFT · 2021-10-06T12:38:45Z

sdk-cpp/include/do_error_macros.h

+#endif
+
+//TODO(jimson) Looks like these conversions are causing test issues with tests
+#define DO_ERROR_FROM_SYSTEM_ERROR(x) (int32_t)(0xC0000000 | (0xD0 << 16) | ((int32_t)(x) & 0x0000FFFF))


0xD0

FACILITY_DELIVERY_OPTIMIZATION #Closed

shishirb-MSFT · 2021-10-06T12:39:39Z

sdk-cpp/include/do_error_macros.h

+#endif
+
+//TODO(jimson) Looks like these conversions are causing test issues with tests
+#define DO_ERROR_FROM_SYSTEM_ERROR(x) (int32_t)(0xC0000000 | (0xD0 << 16) | ((int32_t)(x) & 0x0000FFFF))


DO_ERROR_FROM_SYSTEM_ERROR

Suggest turning both the error translations into inline methods. #Closed

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es31-dont-use-macros-for-constants-or-functions

Won't fix here, will reserve that for when we move these macros into the shared error macro header

shishirb-MSFT · 2021-10-06T12:52:09Z

sdk-cpp/include/do_exceptions.h

@@ -4,10 +4,13 @@
 #ifndef _DELIVERY_OPTIMIZATION_DO_EXCEPTIONS_H


_DELIVERY_OPTIMIZATION_DO_EXCEPTIONS_H

Considering renaming this file now that exceptions can be turned off, to not confuse users. do_errors.h? #Closed

shishirb-MSFT · 2021-10-06T12:52:35Z

#ifndef FAILED

Remove. Defined in do_error_macros.h.

In reply to: 936179229

Refers to: sdk-cpp/include/do_exceptions.h:20 in 57adb40. [](commit_id = 57adb40, deletion_comment = False)

shishirb-MSFT · 2021-10-06T13:00:42Z

sdk-cpp/include/do_download.h

+    void get_property(download_property key, download_property_value& value);
+#endif
+
+    int32_t start_nothrow() noexcept;


int32_t

Highly recommend returning std::error_code with our existing error_category. I don't know all the details of reusing std::error_code correctly but we should get the API right.

Expose a new method in do_exceptions.h, in our details namespace, for use in all our methods:
std::error_code make_error_code(int32_t code); #Closed

We'll probably need other make_*_error_code methods to differentiate errors from different sources (boost/libcurl/OS) but those can come later.

I plan on adding a custom error class in the future PR and returning that custom error code, this was moreso to just get the code factored correctly.

Cool. Was gonna ask whether more PRs are coming.

shishirb-MSFT · 2021-10-06T13:15:59Z

sdk-cpp/src/do_download.cpp

+    else
+    {
+        throw_if_fail(_download->SetProperty(prop, val));
+    }


Please call the nothrow version for non-trivial methods. #Closed

shishirb-MSFT · 2021-10-06T13:17:55Z

sdk-cpp/src/do_download.cpp

        if (isCancelled)
        {
-            msdod::ThrowException(std::errc::operation_canceled);
+            return DO_ERROR_FROM_STD_ERROR(std::errc::operation_canceled);


DO_ERROR_FROM_STD_ERROR

When we return std::error_code instead, we don't have to perform these conversions. Tests will be happy too.

shishirb-MSFT · 2021-10-06T13:20:19Z

sdk-cpp/src/do_download_property.cpp

+    RETURN_IF_FAILED(temp._val->Init(val));
+
+    out = temp;
+    return S_OK;;


;;

nit: double semi-colons #Closed

shishirb-MSFT · 2021-10-06T13:25:14Z

sdk-cpp/src/do_exceptions.cpp

@@ -1,5 +1,4 @@
-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.


Retain copyright. You'll also need to scope the #if when using std::error_code return type. #Resolved

shishirb-MSFT · 2021-10-06T13:31:29Z

sdk-cpp/src/do_exceptions.cpp

@@ -1,5 +1,4 @@
-// Copyright (c) Microsoft Corporation.
-// Licensed under the MIT License.
+#if (DO_ENABLE_EXCEPTIONS)


#if

Use #if defined(DO_ENABLE_EXCEPTIONS). We do not intend to depend on the value of the DO_ENABLE_EXCEPTIONS macro. #Closed

Copyright header is removed in some other files too. Intentional?

No, issue with merge - will resolve

shishirb-MSFT · 2021-10-06T13:31:58Z

sdk-cpp/src/internal/do_noncopyable.h

 // Licensed under the MIT License.

-#ifndef _DELIVERY_OPTIMIZATION_DO_NONCOPYABLE_H
+#ifndef _DELIVERY_OPTIMIZATION_DNONCOPYABLE_H


_DELIVERY_OPTIMIZATION_DNONCOPYABLE_H

revert #Resolved

fyi, not reverted yet

shishirb-MSFT · 2021-10-06T13:32:36Z

sdk-cpp/src/internal/do_exceptions_internal.cpp


 #include "do_exceptions_internal.h"

+#include <exception>


Not required. Header already includes it. #Closed

shishirb-MSFT · 2021-10-06T13:38:54Z

sdk-cpp/src/internal/com/do_download_property_internal.cpp

 using namespace microsoft::deliveryoptimization::details;

-std::wstring UTF8toWstr(const char* str, size_t cch = 0)
+int32_t UTF8toWstr(std::wstring& wstr, const char* str, size_t cch = 0)


wstr

Move wstr to end of params list. #Closed

shishirb-MSFT · 2021-10-06T13:41:28Z

sdk-cpp/src/internal/com/do_download_property_internal.cpp

-CDownloadPropertyValueInternal::CDownloadPropertyValueInternal(const download_property_value::status_callback_t& val)
+int32_t CDownloadPropertyValueInternal::Init(const download_property_value::status_callback_t& val) noexcept
 {
+    V_VT(&_var) = VT_EMPTY;


V_VT(&_var) = VT_EMPTY;

Why not use VariantInit? Standard/conventional way. #Resolved

Since the std::vector one also requires this, I suggest implementing the default constructor and calling VariantInit there. The constructor on the rest side can use = default in its .cpp file.

Fixing in next PR

shishirb-MSFT · 2021-10-06T13:49:53Z

sdk-cpp/src/internal/com/do_download_property_internal.cpp

+#ifdef DEBUG
+    assert(SUCCEEDED(VariantCopy(&_var, &rhs._var)));
+#else:
+    (void)VariantCopy(&_var, &rhs._var);


void

Silently ignores errors. I suggest calling std::terminate instead when this fails (memory alloc failure). #Resolved

Same for other places where we only expect allocation failures.

if (FAILED(res))
{
#if defined(DO_ENABLE_EXCEPTIONS)
throw std::bad_alloc();
#else
std::terminate();
#endif
}

Noted, fixing in next PR

shishirb-MSFT · 2021-10-06T13:56:32Z

sdk-cpp/src/internal/rest/download_impl.cpp

        }
+        return static_cast<int32_t>(msdo::errc::no_service);
+    }
+    catch (msdo::exception& e)


msdo::exception

All invoked methods here throw only this exception type? #Resolved

No, fixing rest side in next PR

Actually the uri builder invoked methods don't throw exceptions as far as I can tell.

shishirb-MSFT · 2021-10-06T14:00:21Z

sdk-cpp/tests/com/download_properties_tests.cpp

    std::vector<int32_t> expectedErrors = { 0, static_cast<int32_t>(msdo::errc::do_e_unknown_property_id) };

-    msdo::download_property_value integrityCheckMandatory(true);
+    std::unique_ptr<msdo::download_property_value> integrityCheckMandatory = std::make_unique<msdo::download_property_value>();


make_unique

Why allocate and then call make() again? #Closed

shishirb-MSFT · 2021-10-06T14:01:39Z

sdk-cpp/tests/com/download_properties_tests.cpp

    ASSERT_TRUE(false);
 }

+// For some reason, custom headers are getting rejected and returning E_INVALIDARG now, disabling test


disabling

Use the gtest way of disabling tests: prefix method name with DISABLED_ #Closed

shishirb-MSFT · 2021-10-06T14:02:58Z

sdk-cpp/src/do_download.cpp

 {
-    _download = std::make_shared<msdod::CDownloadImpl>(uri, downloadFilePath);
+    _download = std::make_shared<msdod::CDownloadImpl>();
+    _download->Init(uri, downloadFilePath);


Init

Ignores errors from the Init method. Do you intend to handle it in a separate PR? #Resolved

Probably should use the make methods convention here as well

Fixing in next PR

shishirb-MSFT · 2021-10-07T04:57:59Z

build/build.py

            generate_options.extend(["-DDO_BUILD_TESTS=OFF"])

+        if self.enable_exceptions:
+            generate_options.extend(["-DDO_ENABLE_EXCEPTIONS=OFF"])


OFF

This should be ON but since that is the default, I'd remove --enable-exceptions arg entirely. #Resolved

We should have a way to disable without modifying source code, im flipping the flag back to disable

shishirb-MSFT

jimson-msft added 3 commits October 2, 2021 12:53

Return error codes internally and provide error code returning interface

d31e1a7

Merge branch 'develop' into user/jimson/error_code_interface

8a35b5e

fix merge issue

400cc7d

jimson-msft requested a review from a team as a code owner October 2, 2021 20:31

add flag to build scripts

57adb40

shishirb-MSFT reviewed Oct 6, 2021

View reviewed changes

jimson-msft added 3 commits October 6, 2021 15:52

resolve pr comments, change name of exception file

2da0835

make methods use return value rather than out param

4679bb6

resolve most pr feedback

1435a75

shishirb-MSFT reviewed Oct 7, 2021

View reviewed changes

shishirb-MSFT approved these changes Oct 7, 2021

View reviewed changes

jimson-msft added 5 commits October 7, 2021 13:28

fix build.py

9986b92

cleanup cmakelists

214f736

adjust pipeline yamls

5ccbae3

fix plugin biuld

48469fe

Adjust binary size

d3f92c2

jimson-msft merged commit 90e4718 into develop Oct 7, 2021

jimson-msft deleted the user/jimson/error_code_interface branch October 7, 2021 23:00

		@@ -4,10 +4,13 @@
		#ifndef _DELIVERY_OPTIMIZATION_DO_EXCEPTIONS_H

		@@ -1,5 +1,4 @@
		// Copyright (c) Microsoft Corporation.
		// Licensed under the MIT License.

Error Code Interface & Partially Remove Internal Exceptions #88

Error Code Interface & Partially Remove Internal Exceptions #88

Uh oh!

Conversation

jimson-msft commented Oct 2, 2021

Uh oh!

shishirb-MSFT Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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 Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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 Oct 6, 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 Oct 6, 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 Oct 6, 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 Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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 commented Oct 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shishirb-MSFT Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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!

Choose a reason for hiding this comment

Uh oh!

shishirb-MSFT Oct 6, 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 Oct 6, 2021 • edited by jimson-msft Loading Uh oh! There was an error while loading. Please reload this page.

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT commented Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading

shishirb-MSFT Oct 6, 2021 •

edited

Loading

shishirb-MSFT Oct 6, 2021 •

edited by jimson-msft

Loading