Make Platform functions to return az_result #1256

antkmsft · 2020-09-11T00:55:53Z

This is variant A of the proposed behavior. Variant B: #1257

ahsonkhan

The other variant is better.

ahsonkhan · 2020-09-11T01:25:32Z

sdk/inc/azure/core/az_platform.h

 */
-void az_platform_sleep_msec(int32_t milliseconds);
+AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds);


This is an unnecessary burden to the implementor of the PAL.

@ahsonkhan are you referring to the AZ_NODISCARD or the az_result. I agree that we shouldn't force AZ_NODISCARD onto the implementations. I however do prefer the az_result return codes on the PAL. The minimal burden is greatly outweighed by the fact we can consistently validate the API performed as expected

are you referring to the AZ_NODISCARD or the az_result

Both.

The minimal burden is greatly outweighed by the fact we can consistently validate the API performed as expected

I don't understand what you mean by this. What are we getting here?

Ha, what I was thinking of, is that you Ahson was talking about the contrast of this variant to the proposal in #1257, saying that out parameters and az_result are generally more cumbersome to implement. I didn't read it as if you are commenting on the AZ_NODISCARD.
Regarding AZ_NODISCARD, I do not think it is a burden on the implementor. It is an intentional burden on the user, i.e. us. So, if we ignore the return result, we get a warning treated as compilation error. For the implementor, I am not sure if they even have to put AZ_NODISCARD in the signature of their imlpementation - having it in this header alone is probably enough (I am not sure, because AZ_NODISCARD is really compiler-specific).

OMHO, platform should not depend on az_core. Only az_core should depend on some specific implementation.
Right now, in order to implement a platform, people need to get az_core, and people getting az_core need an az_platform. I don't like how that look XD. It works using cmake targets, but not sure if it would work without it (including pah directly)

Interesting point on layering the dependency @vhvb1989

ahsonkhan · 2020-09-11T01:26:11Z

sdk/src/azure/platform/az_posix.c

 }

-void az_platform_sleep_msec(int32_t milliseconds)
+AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds)
 {
  (void)usleep((useconds_t)milliseconds * _az_TIME_MICROSECONDS_PER_MILLISECOND);


unrelated: precondition that milliseconds is positive.

So far we say in the docs, @remarks The behavior is undefined when \p milliseconds is a non-positive value (0 or less than 0). That's because on some platform, it would yield to other thread, but will return control back if no other thread needs it. We don't use this feature though.

That's because on some platform, it would yield to other thread, but will return control back if no other thread needs it. We don't use this feature though.

We can add a precondition now to help folks write correct code, and loosen that in the future, if that value becomes meaningful.

That's because on some platform, it would yield to other thread, but will return control back if no other thread needs it. We don't use this feature though.

We can add a precondition now to help folks write correct code, and loosen that in the future, if that value becomes meaningful.

We could precondition, but we would be limiting to a particular implementation of POSIX. Its not clear if every implementation places the same constraints. e.g. some platforms say the usec is not smaller than 1000000 others allow 0 to mean yield.

This is one particular implementation though. Adding a precondition here doesn't impact users who want to provide their own PAL implementation. Passing in a negative millisecond will overflow when cast to unsigned int, and that could be an unintended bug in the user's code (given usleep takes an unsigned number).
https://man7.org/linux/man-pages/man3/usleep.3.html#NOTES

RickWinter · 2020-09-11T16:17:28Z

sdk/inc/azure/core/az_platform.h

 */
-void az_platform_sleep_msec(int32_t milliseconds);
+AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds);


@ahsonkhan are you referring to the AZ_NODISCARD or the az_result. I agree that we shouldn't force AZ_NODISCARD onto the implementations. I however do prefer the az_result return codes on the PAL. The minimal burden is greatly outweighed by the fact we can consistently validate the API performed as expected

sdk/src/azure/core/az_http_policy_logging.c

Co-authored-by: Rick Winter <rick.winter@microsoft.com>

antkmsft · 2020-09-17T00:16:26Z

@ahsonkhan , I pinged Jeffrey, I'll wait to hear his opinion. If you are ok with this implementation, please approve :)

antkmsft · 2020-09-17T01:58:52Z

@ahsonkhan and the team: Jeffrey is ok with returning az_result instead of precondition failure.

ahsonkhan · 2020-09-17T02:14:52Z

instead of precondition failure.

Can we have both?

antkmsft · 2020-09-17T03:51:03Z

instead of precondition failure.

Can we have both?

We could, and I thought about it at some point, but I don't think we should do it, given that precondition failure is not obvious to the users who are not familiar with it, so now that it is failed, users have to go learn about it first, and only then realize they need to implement PAL layer, so it removes one of the benefits of az_result.
Plus, if we are ok with the interface - function signature, returns az_result, precondition failure can be added in the future.

ahsonkhan · 2020-09-17T04:08:31Z

precondition failure can be added in the future.

I would flip that. Precondition failures can be removed in the future. Strengthening a condition is more user hostile than weakening it.

ahsonkhan

We didn't discuss @vhvb1989 point on dependency/layering. I think that is a good argument for not returning az_result. Let's close on that one.

ahsonkhan · 2020-09-17T04:14:40Z

sdk/src/azure/platform/az_win32.c

-void az_platform_sleep_msec(int32_t milliseconds) { Sleep(milliseconds); }
+AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds)
+{
+  Sleep(milliseconds);


I still think we should add a _az_PRECONDITION(milliseconds > 0) to all the implementations of the PAL.

Since we have a variety of opinions, let's cover it outside of the scope of this PR, it can be added later without affecting this change.

ahsonkhan · 2020-09-17T04:16:58Z

sdk/src/azure/platform/az_noplatform.c


-void az_platform_sleep_msec(int32_t milliseconds) { (void)milliseconds; }
+AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds)


Looking at this again, I agree with Rick on the sleep API, we should consider removing AZ_NODISCARD.

I don't think we should. It is us who calls these APIs. Rick's assumption is that 3 things are happening: 1. Customer calls PAL function. 2. Doesn't handle a result, and doesn't need/want to. 3. It is painful to ignore the result, which I think is not:

az_result ignore = az_platform_sleep_msec(1000); (void)ignore;

In that case, we should consider updating the documentation of az_http_pipeline_policy_logging and az_http_pipeline_policy_retry since they can now return any PAL-defined az_result. Alternatively, those APIs should take any PAL-error and convert it to a single error value, and return/document that.

antkmsft · 2020-09-17T04:18:45Z

precondition failure can be added in the future.

I would flip that. Precondition failures can be removed in the future. Strengthening a condition is more user hostile than weakening it.

Jeff said to me "I can appreciate not wanting the customer getting started experience to force understanding on preconditions and figuring out how to deal with an infinite loop. To me, this is the ONLY compelling reason to avoid the precondition. Understanding preconditions is fundamental to our library just like spans and so you're only delaying the customer being forced to understand this; not preventing it entirely."

So I replied "What you've just said about delaying the learning, to me sounds like more of an argument for having the az_result, and a relatively significant one."

Adding a precondition would eliminate one of the benefits we get now.

ahsonkhan · 2020-09-17T04:26:43Z

Adding a precondition would eliminate one of the benefits we get now.

Great point. Progressive disclosure is definitely something we should try to achieve, where possible.

antkmsft · 2020-09-17T04:28:22Z

We didn't discuss @vhvb1989 point on dependency/layering. I think that is a good argument for not returning az_result. Let's close on that one.

We had that discussion about last November with Jeffrey, and decided in favor of header-only what-appears-to-look-like circular dependency, because it is simpler, and it works. I was actually a proponent of breaking the core into two modules back then: small core <- pal <- big core.
Victor's argument can and should be applied to the http adapter implementation, and we're not doing it there. Going with that change implies a lot more than the scope of this PR.

ahsonkhan · 2020-09-17T04:34:33Z

Victor's argument can and should be applied to the http adapter implementation, and we're not doing it there. Going with that change implies a lot more than the scope of this PR.

I think the PAL and HTTP adapter sit at different layers, so I don't think they both need to be held at the same bar.

I was actually a proponent of breaking the core into two modules back then: small core <- pal <- big core.

We shouldn't need a small core below at all for PAL (since conceptually it sits below the SDK, closer to the OS/device hardware):
OS <- pal <- core

antkmsft · 2020-09-17T04:57:04Z

We shouldn't need a small core below at all for PAL (since conceptually it sits below the SDK, closer to the OS/device hardware):
OS <- pal <- core

It was the point that IoT didn't need PAL, and didn't want to depend on it at all so they would depend only on small core. There's even no need to rely on the linker throwing out the unused parts, as today. Small core has spans, results, etc. There was no az_noplatform/az_nohttp in that picture, at all. So, if you are a blob client library, you need big core that has HTTP and PAL dependencies. You on't have a suitable implementation of pal/http - you don't get az_storage, we don't/can't compile it until the dependency is satisfied.

antkmsft · 2020-09-17T05:06:01Z

I think the PAL and HTTP adapter sit at different layers, so I don't think they both need to be held at the same bar.

Why az_curl can have a dependency on az_result, if the PAL can't?
Policies from Core do have http policy, which invokes HTTP adapter, but HTTP adapter relies on Core. It is the same argument.
Current solution: we do it carefully for both cases. If done carefully, it is simpler. Other option: circular2 => linear/diamond3 dependency, as we're discussing with small and big core, but that didn't happen. It is right from every angle, but more massive in terms of understanding/complexity,

ahsonkhan · 2020-09-17T05:22:48Z

Policies from Core do have http policy, which invokes HTTP adapter, but HTTP adapter relies on Core.

True.

@antkmsft

@antkmsft - this change slipped through between the clang format validation check PR and the recent PAL update. #1256 It is blocking CI for other PRs atm.

antkmsft added 2 commits September 10, 2020 17:53

Make Platform functions to return az_result

a88401f

:

97a83a1

antkmsft mentioned this pull request Sep 11, 2020

Invoking functions from az_noplatform fails precondition. #1257

Closed

ahsonkhan suggested changes Sep 11, 2020

View reviewed changes

ahsonkhan assigned antkmsft Sep 11, 2020

ahsonkhan added the Azure.Core label Sep 11, 2020

antkmsft and others added 4 commits September 10, 2020 18:39

Unit test fix

292d43f

Merge branch 'master' into pal-azresult

9ef7685

Mock implementation

74fa1ef

parameter name omitted

94bf74f

antkmsft marked this pull request as ready for review September 11, 2020 09:30

antkmsft requested review from JeffreyRichter, joshfree, RickWinter and vhvb1989 as code owners September 11, 2020 09:30

RickWinter approved these changes Sep 11, 2020

View reviewed changes

antkmsft and others added 2 commits September 11, 2020 10:54

Update sdk/src/azure/core/az_http_policy_logging.c

d67c4a7

Co-authored-by: Rick Winter <rick.winter@microsoft.com>

Update sdk/src/azure/core/az_http_policy_logging.c

be7d438

Co-authored-by: Rick Winter <rick.winter@microsoft.com>

Merge branch 'master' into pal-azresult

c8a87cf

ahsonkhan approved these changes Sep 17, 2020

View reviewed changes

antkmsft closed this Sep 17, 2020

antkmsft reopened this Sep 17, 2020

Merge branch 'master' into pal-azresult

908d9c6

antkmsft merged commit bb4f941 into Azure:master Sep 17, 2020

ahsonkhan mentioned this pull request Sep 17, 2020

Fix spacing on az_posix to satisfy clang format. #1285

Merged

ghost pushed a commit that referenced this pull request Sep 17, 2020

Fix spacing on az_posix to satisfy clang format. (#1285)

52b58af

@antkmsft - this change slipped through between the clang format validation check PR and the recent PAL update. #1256 It is blocking CI for other PRs atm.

antkmsft deleted the pal-azresult branch October 3, 2020 01:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Platform functions to return az_result #1256

Make Platform functions to return az_result #1256

antkmsft commented Sep 11, 2020 •

edited

Loading

ahsonkhan left a comment

ahsonkhan Sep 11, 2020

RickWinter Sep 11, 2020

ahsonkhan Sep 11, 2020 •

edited

Loading

antkmsft Sep 11, 2020

vhvb1989 Sep 14, 2020

ahsonkhan Sep 14, 2020

ahsonkhan Sep 11, 2020

antkmsft Sep 11, 2020

ahsonkhan Sep 11, 2020 •

edited

Loading

RickWinter Sep 11, 2020

ahsonkhan Sep 11, 2020 •

edited

Loading

RickWinter Sep 11, 2020

antkmsft commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

ahsonkhan left a comment

ahsonkhan Sep 17, 2020

antkmsft Sep 17, 2020 •

edited

Loading

ahsonkhan Sep 17, 2020

antkmsft Sep 17, 2020

ahsonkhan Sep 17, 2020 •

edited

Loading

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020 •

edited

Loading

antkmsft commented Sep 17, 2020 •

edited

Loading

antkmsft commented Sep 17, 2020 •

edited

Loading

ahsonkhan commented Sep 17, 2020


		void az_platform_sleep_msec(int32_t milliseconds) { (void)milliseconds; }
		AZ_NODISCARD az_result az_platform_sleep_msec(int32_t milliseconds)

Make Platform functions to return az_result #1256

Make Platform functions to return az_result #1256

Conversation

antkmsft commented Sep 11, 2020 • edited Loading

ahsonkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Sep 11, 2020 • 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

ahsonkhan Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antkmsft commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

ahsonkhan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antkmsft Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020

antkmsft commented Sep 17, 2020

ahsonkhan commented Sep 17, 2020 • edited Loading

antkmsft commented Sep 17, 2020 • edited Loading

antkmsft commented Sep 17, 2020 • edited Loading

ahsonkhan commented Sep 17, 2020

antkmsft commented Sep 11, 2020 •

edited

Loading

ahsonkhan Sep 11, 2020 •

edited

Loading

ahsonkhan Sep 11, 2020 •

edited

Loading

ahsonkhan Sep 11, 2020 •

edited

Loading

antkmsft Sep 17, 2020 •

edited

Loading

ahsonkhan Sep 17, 2020 •

edited

Loading

ahsonkhan commented Sep 17, 2020 •

edited

Loading

antkmsft commented Sep 17, 2020 •

edited

Loading

antkmsft commented Sep 17, 2020 •

edited

Loading