Not receiving async done tag for failed incoming call requests

[jeady]

The comment on "AsyncNotifyWhenDone" states "Has to be called before the rpc starts" but it seems that if the request tag is returned with ok=false (i.e. because the CQ is shutting down) then the async done tag is never received. Instead, I expect the async done tag to be received regardless of whether or not an incoming call request was successfully received.

Worth noting is I have also seen cases where I received both the call request with ok=false and and done tag when there was some error (such as deadline exceeded) and not when the CQ is shutting down.

[ctiller]

Hey @lyuxuan - can you take a first peek at this.

I think we need a test case that:

1. creates a ServerContext and calls AsyncNotifyWhenDone
2. requests a call (using the async server API) using that ServerContext
3. shuts down the server

And I think the expectation is that right now we'll see the tag for (2) come back but not for (1).
We should also see (1).

[lyuxuan]

I add a test case to the async_end2end_test, and indeed (1) does not come back.
#10143

[lyuxuan]

It's blocking internal user

[jeady]

As a side note, the reason we're calling AsyncNotifyWhenDone is because we want to be able to call ServerContext::IsCancelled, which has this comment:

// When using async API, it is only safe to call IsCancelled after
// the AsyncNotifyWhenDone tag has been delivered

We would probably be able to infer the "done-ness" of the connection without this tag otherwise.

[jeady]

Any status update here?

[lyuxuan]

We have a fix plan, and should be able to update it by the end of this week. Thanks!

[abaldeva]

This issue is still present in 1.4.2. Any updates?

[lyuxuan]

Fix is in progress. #11955

[vjpai]

Is this issue still alive or is it closed?

[ctiller]

It's alive: @sreecha was going to look at it in the next week or so.

…

On Tue, Oct 10, 2017 at 11:07 PM Vijay Pai ***@***.***> wrote: Is this issue still alive or is it closed? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#10136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJpudbEIvjobSrrGSMqN1VDH2m6ncrnBks5srFsEgaJpZM4Mbv4O> .

[muxi]

ping?

[abaldeva]

Are there any plans for fixing this issue? For us, this results in lot of memory leaks at shutdown which screws up our memory leak validation tool chain.

Thanks.

[sreecha]

Hi @abaldeva , I looked at it a few months ago and realized that the fix is very invasive and requires a lot of rewiring the code. It is currently in the back-burner. I am going through all our open issues this week; I'll give you a more concrete answer mid to late week.

[AspirinSJL]

I also saw this issue when working on #15853. I will add some comment to that API so that users are warned about this issue before using that API.

[mehrdada]

Past the two year mark and still an open issue. Pretty annoying as it seems to be the only exception to "whatever you feed the CQ, you'll get back" invariant.

[stale]

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 180 days. It will be closed automatically if no further update occurs in 1 day. Thank you for your contributions!

[mehrdada]

Issue definitely persists -- reopening

[stale]

This issue/PR has been automatically marked as stale because it has not had any update (including commits, comments, labels, milestones, etc) for 30 days. It will be closed automatically if no further update occurs in 7 day. Thank you for your contributions!

[xiedeacc]

is this issue still exists?

[adrianimboden]

Yes, it is still an issue. I also have to disable leak checking for most tests because of this now.

Sadly, as I understand it, from a user point it is unavoidable: You have to call context.AsyncNotifyWhenDone before the request starts. If it never starts it is also not safe to delete the tag myself as it sometimes seems to get delivered anyway?

Disabling leak checking in those tests seems to be easiest.

I assume the reason for not fixing this is that most applications don't start/stop the servers all the time on production.

[yoonseok-kim]

I come up with a workaround to solve this problem at the application level. Depending on whether rpc initiate is in progress, it is determined whether to free the (dynamically allocated) tag to be delivered to AsyncNotifyWhenDone.

I wonder if this method is safe?

class CallDataInterface {
public:
    virtual ~CallDataInterface() = default;
    virtual void Proceed() = 0;
};

class DoneCallData : public CallDataInterface {
public:
    DoneCallData() = default;
    ~DoneCallData() = default;

    void Proceed() override { delete this; }
};

class SayHelloCallData : public CallDataInterface {
   public:
    SayHelloCallData(Greeter::AsyncService* service, ServerCompletionQueue* cq)
        : service_(service), cq_(cq), responder_(&ctx_), status_(CREATE) {
      initiate_ = false;
      done_ = std::make_unique<DoneCallData>();
      Proceed();
    }

    ~SayHelloCallData() {
        if (initiate_) {
            done_.release();
        }
    }

    void Proceed() override {
      if (status_ == CREATE) {
        status_ = PROCESS;
        ctx_.AsyncNotifyWhenDone(done_.get());
        service_->RequestSayHello(&ctx_, &request_, &responder_, cq_, cq_,
                                  this);
      } else if (status_ == PROCESS) {
        initiate_ = true;
        new SayHelloCallData(service_, cq_);

        std::string prefix("Hello ");
        reply_.set_message(prefix + request_.name());

        status_ = FINISH;
        responder_.Finish(reply_, Status::OK, this);
      } else {
        GPR_ASSERT(status_ == FINISH);
        delete this;
      }
    }

   private:
    bool initiate_;
    std::unique_ptr<DoneCallData> done_;
    Greeter::AsyncService* service_;
    ServerCompletionQueue* cq_;
    ServerContext ctx_;

    HelloRequest request_;
    HelloReply reply_;

    ServerAsyncResponseWriter<HelloReply> responder_;

    enum CallStatus { CREATE, PROCESS, FINISH };
    CallStatus status_;
  };

(written with 'grpc helloworld async server example'.)

[brunexgeek]

Sadly, as I understand it, from a user point it is unavoidable: You have to call context.AsyncNotifyWhenDone before the request starts. If it never starts it is also not safe to delete the tag myself as it sometimes seems to get delivered anyway?

In my implementation, I used a memory pool for managing call data objects. Rather than allocating memory for each individual object, I used a preallocated buffer from the memory pool and instantiate with placement new (e.g. new (buffer) CallData()). If the notification never arrives, the buffer remains reserved in the pool, although "wasted". However, I have the flexibility to deallocate these buffers either upon stopping the server or dynamically during execution (e.g. employing a timeout mechanism).

[markdroth]

This is a known deficiency of the CQ-based async API. At this point, we would recommend users migrate to the newer callback-style API instead.