Add Operation<T>

[Rick Winter (RickWinter)]

Add support for Operation

[check-enforcer]

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run cpp - [service] - ci

[Victor Vazquez (vhvb1989)]

Looks good.

We could consider creating a simple service somewhere to tests this with network

[Victor Vazquez (vhvb1989)]

I have asked Anton Kolesnyk (@antkmsft) what's the difference between
explicit LongRunningOperationState(const std::string& value) : m_value(value)
vs
explicit LongRunningOperationState(std::string value) : m_value(std::move(value))

For what I understand, the second way provides a better and clearer intention about the method will make a copy of the string that we are passing.
While the first one, seems like the method is only reading the value (and we make a copy of it behind the scenes)

So, the preferred way should be the second approach

[Ahson Khan (ahsonkhan)]

Keep in mind, these are public APIs so we should try to be idiomatic and avoid unnecessary string copies.

[Ahson Khan (ahsonkhan)]

Add doxygen docs and maybe add more test cases, especially around the edges.

[Ahson Khan (ahsonkhan)]

Is this a common pattern to have private virtual methods suffixed with Internal?

Is this API accessible to outside of the Azure::Core package? Who will be able to derive from Operation and override PollInternal and who will be able to call these?
I am guessing that no one outside Azure::Core can call these methods, but any derived class can override them (even end customer code). Is that correct?

Would we want such APIs to show up in APIView?

[Rick Winter (RickWinter)]

They are private methods in the class. There is no accessibility to them so naming isn't dictated other than Pascal

[Ahson Khan (ahsonkhan)]

Why are they pure virtual then if they aren't mean to be accessible outside the class?

http://www.gotw.ca/publications/mill18.htm

Guideline #2: Prefer to make virtual functions private.
This lets the derived classes override the function to customize the behavior as needed, without further exposing the virtual functions directly by making them callable by derived classes (as would be possible if the functions were just protected). The point is that virtual functions exist to allow customization; unless they also need to be invoked directly from within derived classes' code, there's no need to ever make them anything but private.

From https://stackoverflow.com/a/2170696/12509023

Also from https://stackoverflow.com/a/3978552/12509023

The confusion is common enough, that C++ FAQ advocated against using private virtuals, for a long time, because confusion seemed to be a bad thing.

Though I can't find mention of that in the provided link. Casey Carter (@CaseyCarter) - thoughts?

[Jeffrey Richter (JeffreyRichter)]

private & virtual doesn't make sense to me. Pure makes less sense as a derived class must implement it but it can't because it's private.
Maybe I'm missing something specific to C++ here, but C# wouldn't even compile this.

[Rick Winter (RickWinter)]

Operation is a template. The derived type must provide an implementation for these two functions. They are private b/c we do not want them directly called. Instead it is the overloads on Operation which will call the derived types methods.

[Ahson Khan (ahsonkhan)]

Making it private restricts even derived types from calling it, which has some value. They can override the behavior, but when and how it is called is controlled by the base type. That can be a useful mechanism to scope and isolate what the derived types can do.

The problem I see is, currently we aren't doing anything significant to control that behavior in some way (like checking context/etc.) and end up just calling their overridden method directly without any prologue/epilogue ceremony at the call site.

[Jeffrey Richter (JeffreyRichter)]

I do not understand what private, virtual means. Private normally means that the member is accessible onto the class itself and virtual means that it is overridable by a derived type. You can't have a derived type access a member that is not accessible.

Furthermore, making it pure (=0) means that the derived type MUST implement something that it can't access. I'm actually surprised that this compiles.

Casey Carter (@CaseyCarter), am I misunderstanding something here?

[Victor Vazquez (vhvb1989)]

Making it private restricts even derived types from calling it

That's partially true. The derived class can only call its own implementation.

but when and how it is called is controlled by the base type.

No, the derived class can call its own implementation whenever it wants. You can have this:

class Base {
private:
  virtual int Foo() = 0;
};

class Vh : public Base {
private:
  int Foo() override { return 1; }

public:
  // As derived class, I can call my implementation whenever I want
  int DoSomething() { return Foo(); }
};

The only thing different from making the pure virtual method protected and not private is to block derived classes from calling the virtual base function directly. But this would make no sense at all!
Like

class Base {
protected:
  virtual int Foo() = 0;
};

class Vh : public Base {
private:
  int Foo() override { return 1; }

public:
  // There would have no sense to call the base to redirect to my own implementation
  int DoSomething() { return Base::Foo(); }
};

[Anton Kolesnyk (antkmsft)]

Private virtual is a legit C++ concept called NVI (non-virtual interface), Herb Sutter is a huge fan.

[Jeffrey Richter (JeffreyRichter)]

Thanks Anton. I found this: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Non-Virtual_Interface
So now I see that this is legit C++ but I still don't fully understand. Why not just make the methods protected instead of private?

[Jeffrey Richter (JeffreyRichter)]

private & virtual doesn't make sense to me. Pure makes less sense as a derived class must implement it but it can't because it's private.
Maybe I'm missing something specific to C++ here, but C# wouldn't even compile this.

[Jeffrey Richter (JeffreyRichter)]

Rick & I reversed the params to make context be optional but that may no longer apply now if an overload that doesn't require context was added.

[Jeffrey Richter (JeffreyRichter)]

Yes. It's OK if we want to call it ID but token is probably a better name.

[Jeffrey Richter (JeffreyRichter)]

Here's why this function is required: A machine initiates the LRO and then can get a token (or ID), pass the token(Id) to another machine. The other machine can use its XxxClient and call an overload of the StartXxx method that accepts the token/id. This overload also returns an Operation and now the other machine can poll until done.

[Ahson Khan (ahsonkhan)]

Blocking until we have doxygen docs, unless you plan to add those later.

[Ahson Khan (ahsonkhan)]

nit: fully qualify the type

Suggested change

	- Added support for long-running operations with `Operation<T>`.
	- Added support for long-running operations with `Azure::Core::Operation<T>`.

[Anton Kolesnyk (antkmsft)]

I don't think we should do it, it is less readable. Unless we move things from namespace to namespace.

[Ahson Khan (ahsonkhan)]

I don't understand why we are accepting context if we are not going to do anything with it. We should consider checking or context.IsCanceled() and setting the operationstatus to cancelled before calling PollInternal and not call it for such cases.

Is the point that context is just a pass through and it's the implementer's responsibility to do with it what they wish? Is there any reason why they wouldn't honor context.IsCanceled()?

I don't know what we'd do for returning the raw response type in that case, but something like:

    std::unique_ptr<Http::RawResponse> Poll(Context& context)
    {
      if (context.IsCanceled())
      {
        m_status = OperationStatus::Cancelled;
        return ???; // or do we still end up calling the user function? both are odd
        // OR
        context.ThrowIfCanceled();
      }
      return PollInternal(context);
    }

Same approach for PollUntilDone.

[Rick Winter (RickWinter)]

This is a Mocked up Operation for test purposes. In real implementations the PollInternal method would trigger the pipeline via the client, i.e. pipeline->Send(context, ...._) method. The pipeline is the place we check the context and the pipeline throws if the context is cancelled.

What I've written for the test is to eliminate the client and pipeline from my test implementation of Operation. They are extra dependencies that the test doesn't need to validate an Operation builds, runs, functions as expected

[Jeffrey Richter (JeffreyRichter)]

The Poll function makes an I/O request to the service. ALL service I/O requests take a context and honor it already. So, the derive type that implements Poll will take the Context and pass it to the pipeline which will honor it; there is no need for anything in Operation to check the Context.

In short, the context should be checked immediately before an I/O operation and we have 2 kinds of I/O operations. Just before making an HTTP request and we're going to have the Transport policy check the context for this. The other is when reading from a BodyStream and we're going to have BodyStream's Read method do this check.

We should not be checking context cancellation anywhere else in the SDK.
Someday, we might have 1 exception to this: we might check Cancellation in a long-running compute-bound loop that does no I/O operations. Today, we don't have any of these I don't think we ever will.

[Ahson Khan (ahsonkhan)]

The pipeline is the place we check the context and the pipeline throws if the context is cancelled.

The Poll function makes an I/O request to the service. ALL service I/O requests take a context and honor it already. So, the derive type that implements Poll will take the Context and pass it to the pipeline which will honor it; there is no need for anything in Operation to check the Context.

This is a really important design detail for folks like me to understand the relationship between context and operation. Thank you!

I am definitely interested in seeing how the concrete derived types end up looking like in the client library and how this all flows through the SDK.

[Ahson Khan (ahsonkhan)]

Fixes #531

[Ahson Khan (ahsonkhan)]

Some docs suggestions like a -> an, or add a period.
Looks good otherwise.

[Ahson Khan (ahsonkhan)]

Can we consider something like this so that all implementations of Value in the derived types don't have to worry about providing error handling, and we can have a consistent exception type/message?

    private virtual T ValueImplementation() const = 0;
    T Value() const
    {
      if (!HasValue())
      {
        throw some exception
      }
      return ValueImplementation();
    }

[Victor Vazquez (vhvb1989)]

Think we can also do this for Nullable<>::GetValue()

[Ahson Khan (ahsonkhan)]

This fits well as a precondition check so it can be turned off in release once the calling code is correctly written to avoid the unnecessary cost of extra checks