thejinchao / turbolink

TurboLink is an unreal engine plugin enables Google gRPC work with Unreal Engine using C++ and Blueprint
MIT License
151 stars 46 forks source link

Error handling #31

Open MarcoForlivesi opened 10 months ago

MarcoForlivesi commented 10 months ago

If an error happens the plugin doesn't provide any event

savasozer commented 9 months ago

You can use FGrpcResult.Result.Code in response lambda to check occurred an error

MarcoForlivesi commented 9 months ago

@savasozer in case of error, the lambda function is not reached.

template<typename T, typename R>
class GrpcContext_Ping_Stream : public TGrpcContext<T, R>
{
    typedef TGrpcContext<T, R> Super;
protected:
    void OnRpcEventInternal(bool Ok, const void* EventTag, typename Super::FRpcCallbackFunc RpcCallbackFunc)
    {
        if (!Ok)
        {
            Super::RpcReaderWriter->Finish(&(Super::RpcStatus), Super::ReadTag);
            Super::UpdateState(EGrpcContextState::Done);
            return;
        }

As a test I deleted the authorization token from my service, in case of error it seems to go through here (TurboLinkGrpcContext.h), but the callback is not called and for some reason Super::RpcStatus turns out to be ok

The rpc method is: rpc FromTextToVoice (TextData) returns (stream ResponseData);

thejinchao commented 9 months ago

Sorry, I'm very busy with work recently and don't have time to take care of this project. I will take a look at this issue when I have time.

6r0m commented 8 months ago

@thejinchao @MarcoForlivesi

I think I realized how it should work in Turbolink context)

  1. when Ok=false we shouldn't call Super::UpdateState(EGrpcContextState::Done) in the OnRpcEventInternal (TurboLinkGrpcContext.h). because we need to wait when Super::RpcReaderWriter->Finish will be processed (another AsyncNext in the TurboLinkGrpcManager.cpp). only after next processing, proper status will be written in relevant context.

also we need to implement another tag for filtering the Finish call

if (!Ok)
{
    Super::RpcReaderWriter->Finish(&(Super::RpcStatus), Super::FinishTag);

    // Don't Update State to Done here, because need to wait when this operation finished on the next AsyncNext which updates Super::RpcStatus
    // Super::UpdateState(EGrpcContextState::Done);
    return;
}
  1. on the next AsyncNext you will receive updated grpcContext with relative RpcStatus with code_ and errormessage

  2. finally, in the OnRpcEventInternal (which will be called after AsyncNext in the relative context): we need to filter FinishTag after updating status and in case error push callback with wrong status

// Finish operation processed - handle error if exists and Update State to Done
if (EventTag == Super::FinishTag)
{           
    if (RpcCallbackFunc && !Super::RpcStatus.ok())
    {
        UE_LOG(LogTurboLink, Error, TEXT("CallRpcError: %s"), *result.GetMessageString());

        if (RpcCallbackFunc)
        {
            RpcCallbackFunc(result, nullptr);
        }
    }

    Super::UpdateState(EGrpcContextState::Done);
    return;
}

also, in case if we sure that receiving wrong status only possible after AsyncNext isn't Ok then could omit further else branch `if (Super::RpcStatus.ok())

slightly modified GrpcContext_Ping_Stream example:

template<typename T, typename R>
class GrpcContext_Ping_Stream : public TGrpcContext<T, R>
{
    typedef TGrpcContext<T, R> Super;
protected:
    void OnRpcEventInternal(bool Ok, const void* EventTag, typename Super::FRpcCallbackFunc RpcCallbackFunc)
    {
        if (!Ok)
        {
            Super::RpcReaderWriter->Finish(&(Super::RpcStatus), Super::FinishTag);

            // Don't Update State to Done here, because need to wait when this operation finished on the next AsyncNext which updates Super::RpcStatus
            // Super::UpdateState(EGrpcContextState::Done);
            return;
        }

        FGrpcResult result = GrpcContext::MakeGrpcResult(Super::RpcStatus);

        // Finish operation processed - handle error if exists and Update State to Done
        if (EventTag == Super::FinishTag)
        {           
            if (RpcCallbackFunc && !Super::RpcStatus.ok())
            {
                UE_LOG(LogTurboLink, Error, TEXT("CallRpcError: %s"), *result.GetMessageString());

                if (RpcCallbackFunc)
                {
                    RpcCallbackFunc(result, nullptr);
                }
            }

            Super::UpdateState(EGrpcContextState::Done);
            return;
        }

        // In case if we sure that receiving wrong status only possible after AsyncNext isn't Ok then could omit further else branch
        if (Super::RpcStatus.ok())
        {
            if (Super::GetState() == EGrpcContextState::Initialing)
            {
                check(EventTag == Super::InitialTag);

                Super::RpcReaderWriter->Read(&(Super::RpcResponse), Super::ReadTag);
                Super::UpdateState(EGrpcContextState::Busy);
            }
            else
            {
                if (RpcCallbackFunc)
                {
                    RpcCallbackFunc(result, &(Super::RpcResponse));
                }
                Super::RpcReaderWriter->Read(&(Super::RpcResponse), Super::ReadTag);
            }
        }
        else
        {
            UE_LOG(LogTurboLink, Error, TEXT("CallRpcError: %s"), *result.GetMessageString());

            if (RpcCallbackFunc)
            {
                RpcCallbackFunc(result, nullptr);
            }
            Super::UpdateState(EGrpcContextState::Done);
            return;
        }
    }
public:
    GrpcContext_Ping_Stream(FGrpcContextHandle _Handle, UGrpcService* _Service, UGrpcClient* _Client)
        : Super(_Handle, _Service, _Client)
    {
    }
};

thanks again for the plugin - it's a lifesaver!

MarcoForlivesi commented 7 months ago

Yes, it seems works, but you miss the part of the tag handling

    //async tag
    void* InitialTag = nullptr;
    void* WriteTag = nullptr;
    void* ReadTag = nullptr;
    void* FinishTag = nullptr;
void GrpcContext::UpdateState(EGrpcContextState NewState)
{
    if (GetState() == NewState) return;

    ContextState = NewState;
    if (ContextState == EGrpcContextState::Initialing)
    {
        InitialTag = Service->TurboLinkManager->GetNextTag(AsShared());
        WriteTag = Service->TurboLinkManager->GetNextTag(AsShared());
        ReadTag = Service->TurboLinkManager->GetNextTag(AsShared());
        FinishTag = Service->TurboLinkManager->GetNextTag(AsShared());
    }
    else if (ContextState == EGrpcContextState::Done)
    {
        Service->TurboLinkManager->RemoveTag(InitialTag);
        Service->TurboLinkManager->RemoveTag(WriteTag);
        Service->TurboLinkManager->RemoveTag(ReadTag);
        Service->TurboLinkManager->RemoveTag(FinishTag);
    }

    if (Client->OnContextStateChange.IsBound())
    {
        Client->OnContextStateChange.Broadcast(Handle, NewState);
    }
}
6r0m commented 7 months ago

yeap, you are right, I forget to note this. mostly to emphasize the async behavior.

also in this case, need to add logic above to another context behaviors

MarcoForlivesi commented 5 months ago

@thejinchao is it possible to integrate the changes proposed by 6r0m?

bmurray commented 4 months ago

I was beating my head against a wall trying to figure out what I was doing wrong, only to spend hours going down this rabbit hole. I don't know C++ GRPC very well, but this does seem like the best solution. Is someone working on a PR? That's probably the easiest way to get this fixed. I have another couple of bugs to report too, but those have (sort of) easy workaround. This doesn't.