[Prototype] Implement createchannelasync for requesting channels #508

sharath2727 · 2021-03-02T19:08:46Z

Issue:
As part of the Reunion effort, introduce retry channel request logic within the CreateChannelAsync API to be able to handle benign channel request failures on behalf of the developer.

Description:
As of today, we believe when we fail to fetch channel requests due to errors like ERROR_TIMEOUT etc.., we are throwing exceptions right away and propagating back to the user. These are benign errors and gives us the flexibility to retry channel requests and avoid throwing exceptions for such cases. So for reunion implement a retry logic to request channels on behalf of app developers when we see mentioned errors. This will reduce the number of exceptions we propagate back to the app developers and improves the success rate for channel requests for the caller.

retry logic

dev/PushNotifications/PushManager.cpp

dev/PushNotifications/PushManager.h

dev/PushNotifications/ChannelResult.h

DrusTheAxe · 2021-03-03T07:41:58Z

dev/PushNotifications/ChannelResult.h

+    private:
+        Windows::Networking::PushNotifications::PushNotificationChannel m_channel{ nullptr };
+        winrt::hresult m_extendedError;
+        Microsoft::ProjectReunion::ChannelStatus m_channelStatus;


ProjectReunion shouldn't be used in any symbols or code references.

What's the target namespace? Please update to Microsoft::Something::Something::ChannelStatus

Sure will address this appropriately. Thanks.

@DrusTheAxe I'm aligned with you here, but WinUI 3 is going GA with Microsoft.ProjectReunion namespace prefix. I think the rules changed overnight?

WinUI3 is Microsoft.UI.Xaml - we just had a namespace naming discussion internally, and will publish guidance here soon. The short version is that this should become Microsoft.Windows.PushNotification.Something ... the Microsoft.ProjectReunion namespace was purely a placeholder that got out of hand from my initial commit. :) (Notably, we hadn't had a conversation about Microsoft.ProjectReunion initially; so view this as going from "no rules" to "some rules" overnight.)

That'll be part of the API design & review guidelines that get published, probably early next week at this point.

Jon, thanks for clearing up the namespace related concerns. Once the spec is approved, I would be changing the namespace appropriately. Till then this PR would only be targetted to be part of our private branch WNP_Reunion and not the main. Thanks.

@DrusTheAxe - The spec is being done by Pavan which is currently going through the spec review process. I will be using that spec details to make changes to my code for namespaces.

Not namespace. Package name

Given that Mr. Gallo was in the discussion last night and approved of the guidelines I'll be publishing, yes. :)

There may be a short period of time where the guidance is being rolled out and people move their code around. I recognize that's disruptive, so we want to get it sorted before 0.8, at least. @MikeHillberg or @jevansaks can comment on the WinUI3 namespaces, but I'm pretty sure they've always been Microsoft.UI.* and will continue to be.

Maybe the confusion is that WinUI3 will be delivered via the Project Reunion vehicle that @EHO-makai and company are making steady progress on. We intend Project Reunion as a delivery mechanism for lots of interesting types in the coming months. WinRT and the framework package system we're using lets us be flexible on mapping typenames to implementation DLLs.

For reader context: I quickly pulled my comment after posting (asking Jon to double-check) but I was too slow; two folks replied. 👀

#458 - this is the spec for reference. Thanks.

dev/PushNotifications/ChannelResult.h

DrusTheAxe · 2021-03-03T07:45:14Z

dev/PushNotifications/PushManager.cpp

+using namespace winrt;
+
+// MAX BACKOFF TIME IS 16MINUTES 
+#define MAX_BACKOFF_SECONDS 960


Why #define? Why not e.g. inline constexpr std::uint32_t c_maxBackoffSeconds{ 960 }; ?

Bonus points for putting that inside a namespace e.g.

namespace winrt::Windows::Networking::PushNotifications { inline constexpr std::uint32_t c_maxBackoffSeconds{ 960 };

Since this is a time, maybe use the time constants and time-related functions.

the co_await: co_await winrt::resume_after(std::chrono::seconds(backOffTimeInSeconds)); this takes care of it here. I think this should suffice here: inline constexpr std::uint32_t c_maxBackoffSeconds{ 960 }; ?? please correct me if I am missing something here.

DrusTheAxe · 2021-03-03T07:47:09Z

dev/PushNotifications/PushManager.cpp

+#define WNP_E_RECONNECTING  0x880403E9L
+
+// Error code - Current bind operation failed because another bind operation in progress
+#define WNP_E_BIND_USER_BUSY 0x880403FEL


Are these HRESULTs defined by winerror.h? Are they new constants not in the SDK we're currently compiling against?

@jonwis We have some internal error codes(not part of winerrors.h) that we need to retry on. How do we expose this in Reunion? Is there a common header that we could use or do we go with our own header?

Just declare them here as you have them, but use the MAKE_HRESULT macro. No need to find or add some global common header. If consumers of the API are expected to do something interesting with them, add specific Status codes to the result object that clarify what happened.

dev/PushNotifications/PushManager.cpp

DrusTheAxe · 2021-03-03T07:57:31Z

dev/PushNotifications/PushManager.cpp

+        winrt::Microsoft::ProjectReunion::ChannelResult channelResult{ nullptr };
+        auto progress{ co_await winrt::get_progress_token() };
+
+       GUID remoteIdAbiGuid = reinterpret_cast<GUID&>(remoteId);


remoteId is a winrt::guid. That's not the same memory layout as a GUID&

Why do you need to cast remoteId to GUID?
Can lines 57-58 be replaced by

if (remoteId == winrt::guid{})

?

This whole PR was meant to be a prototype to see if I am able to request channel. Maybe I should have been clear with the PR heading and must have created a draft. This will change to using winrt::guid and will throw is guid is null as it is considered a critical failure :)

GitHub doesn't do 'draft' PRs (not that I've found so far; do speak up if you figure out how)

@jonwis @EHO-makai @BenJKuhn do we have guidance how we should handle 'draft' code reviews? A branch lacks sharing/comment support so a PR's superior to get feedback. If I make a 3-level branching structure main->foo->foo_x and make PRs for foo_x->foo what's the review expectations? Standard review intensity for L3->L2 as L2->L1 (main)? And when the L2 is finally deemed ready to move to L1, do the same review standards apply? As a practical matter, that can lead to very large L2 PRs (harder and more costly to review), but a rubberstamp flush L2->L1 is bad if the L3->L2 review doesn't have the same strict level of review attention and intensity as going into main "because it's just a draft" etc.

There are potential differences here vs how it's handled in other environments. We should have a clear definition of expectations, or folks will do what they've done before (reasonably) assuming that's the way.

Draft PRs were introduced a year+ ago. https://github.blog/2019-02-14-introducing-draft-pull-requests/

Super! Thanks for the pointer

You can convert a Draft PR to not-Draft. Can you go the other way? 'downgrade' a PR to draft? I don't see it mentioned

I don't have appropriate permissions, but you should see this under Reviewers area in top-right corner:

@DrusTheAxe Sure. I have converted this to a draft. I think I agree to the fact that PR review L3->L2 and L2->L1 should have the same review standards (if it is not a draft). Can we have some guidelines on the process so it makes it easier for the developers to follow that approach going ahead.

dev/PushNotifications/PushManager.cpp

dev/PushNotifications/PushManager.h

dev/PushNotifications/PushManager.cpp

hulumane · 2021-03-03T23:16:08Z

dev/PushNotifications/PushManager.cpp

+       {
+           try
+           {
+               auto pushChannel = co_await channelManager.CreatePushNotificationChannelForApplicationAsync();


This API call will fail for Unpackaged apps. Recommendation is to have a check somewhere to see if the solution is packaged or not and maybe fail the API for unpackaged. We should fix this for V1 though when we call into Registration private APIs for unpackaged apps

Note: When you call into the Private APIs for unpackaged apps, you would need to set the PFN to remoteId so that we send it across the wire. That should be supported in downlevel OS's too.

bool IsPackagedProcess() { UINT32 n = 0; const auto rc{ GetCurrentPackageFullName(&n, nullptr) }; THROW_WIN32_ERROR_IF_MSG(HRESULT_FROM_WIN32(rc), (rc != APPMODEL_ERROR_NO_PACKAGE) && (rc != ERROR_INSUFFICIENT_BUFFER), "GetCurrentPackageFullName rc=%d", rc); return rc == ERROR_INSUFFICIENT_BUFFER; } bool IsPackagedProcess_failfast() { UINT32 n = 0; const auto rc{ GetCurrentPackageFullName(&n, nullptr) }; FAIL_FAST_WIN32_ERROR_IF_MSG(HRESULT_FROM_WIN32(rc), (rc != APPMODEL_ERROR_NO_PACKAGE) && (rc != ERROR_INSUFFICIENT_BUFFER), "GetCurrentPackageFullName rc=%d", rc); return rc == ERROR_INSUFFICIENT_BUFFER; }

hulumane · 2021-03-04T02:13:33Z

dev/PushNotifications/PushManager.cpp

+            return false;
+        }
+    }
+
    Windows::Foundation::IAsyncOperationWithProgress<Microsoft::ProjectReunion::ChannelResult, Microsoft::ProjectReunion::ChannelResult> PushManager::CreateChannelAsync(winrt::guid remoteId)


Can we verify the implications of lifetime management? What would happen if the process exited but this AsyncProgress operation is not yet complete? Is the process termination graceful?

https://en.cppreference.com/w/cpp/language/coroutines - When the coroutine state is destroyed either because it terminated via co_return or uncaught exception, or because it was destroyed via its handle, it does the following:

calls the destructor of the promise object.
calls the destructors of the function parameter copies.
calls operator delete to free the memory used by the coroutine state
transfers execution back to the caller/resumer.

So the coroutines completes gracefully.

Also it is important that the caller of the coroutine API: CreateChannelAsync will have to call IAsyncOperationWithProgress.Cancel to stop the asynchronous operation before the caller process exits. We would be clearly mentioning this in the sample that we are providing to cancel the asyncoperation if you are going out of scope. Thanks.

dev/PushNotifications/PushManager.cpp

sharath2727 · 2021-03-19T01:20:39Z

As the spec changed. I am closing this branch and raising a new draft PR for the new branch.

Implement createchannelasyn for requesting channels using

78714ec

retry logic

sharath2727 requested review from danielayala94, hulumane and pmpurifoy March 2, 2021 19:08

ghost added the needs-triage label Mar 2, 2021

hulumane reviewed Mar 2, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Outdated Show resolved Hide resolved

hulumane reviewed Mar 2, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Outdated Show resolved Hide resolved

riverar reviewed Mar 3, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Outdated Show resolved Hide resolved

dev/PushNotifications/PushManager.cpp Outdated Show resolved Hide resolved

dev/PushNotifications/PushManager.h Outdated Show resolved Hide resolved

DrusTheAxe reviewed Mar 3, 2021

View reviewed changes

DrusTheAxe mentioned this pull request Mar 3, 2021

Add note about catching exceptions in C++ #514

Merged

hulumane reviewed Mar 3, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Outdated Show resolved Hide resolved

hulumane reviewed Mar 3, 2021

View reviewed changes

sharath2727 changed the title ~~Implement createchannelasync for requesting channels~~ [Prototype] Implement createchannelasync for requesting channels Mar 4, 2021

hulumane reviewed Mar 4, 2021

View reviewed changes

BenJKuhn removed the needs-triage label Mar 4, 2021

Address comments and also introduce PushNotificationErrorcodes.h

1a35ff2

hulumane reviewed Mar 5, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Show resolved Hide resolved

hulumane reviewed Mar 5, 2021

View reviewed changes

dev/PushNotifications/PushManager.cpp Show resolved Hide resolved

sharath2727 marked this pull request as draft March 5, 2021 21:43

riverar mentioned this pull request Mar 5, 2021

Discussion: Don't add "ProjectReunion" to the Microsoft.WinUI package name microsoft/microsoft-ui-xaml#4427

Closed

sharath2727 added 2 commits March 7, 2021 19:22

Lock only the remoteIdList

3c6e8bc

Make error codes internal to pushnotifications

cf7f238

sharath2727 closed this Mar 19, 2021

bpulliam deleted the user/vemancha/retrylogicchannelrequest branch February 6, 2023 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Prototype] Implement createchannelasync for requesting channels #508

[Prototype] Implement createchannelasync for requesting channels #508

sharath2727 commented Mar 2, 2021

DrusTheAxe Mar 3, 2021

sharath2727 Mar 4, 2021

riverar Mar 5, 2021

jonwis Mar 5, 2021

sharath2727 Mar 5, 2021

sharath2727 Mar 5, 2021

dotMorten Mar 5, 2021

jonwis Mar 5, 2021

riverar Mar 5, 2021

sharath2727 Mar 5, 2021

DrusTheAxe Mar 3, 2021

jonwis Mar 3, 2021

sharath2727 Mar 3, 2021

DrusTheAxe Mar 3, 2021

hulumane Mar 5, 2021

jonwis Mar 5, 2021

DrusTheAxe Mar 3, 2021

sharath2727 Mar 4, 2021

DrusTheAxe Mar 5, 2021 •

edited

Loading

riverar Mar 5, 2021 •

edited

Loading

DrusTheAxe Mar 5, 2021 •

edited

Loading

riverar Mar 5, 2021 •

edited

Loading

sharath2727 Mar 5, 2021

hulumane Mar 3, 2021

hulumane Mar 3, 2021

DrusTheAxe Mar 5, 2021

hulumane Mar 4, 2021

sharath2727 Mar 5, 2021

sharath2727 commented Mar 19, 2021

[Prototype] Implement createchannelasync for requesting channels #508

[Prototype] Implement createchannelasync for requesting channels #508

Conversation

sharath2727 commented Mar 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrusTheAxe Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

riverar Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

DrusTheAxe Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

riverar Mar 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharath2727 commented Mar 19, 2021

DrusTheAxe Mar 5, 2021 •

edited

Loading

riverar Mar 5, 2021 •

edited

Loading

DrusTheAxe Mar 5, 2021 •

edited

Loading

riverar Mar 5, 2021 •

edited

Loading