Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle k_EMsgDestJobFailed #171

Open
Netshroud opened this issue Oct 18, 2015 · 3 comments
Open

Handle k_EMsgDestJobFailed #171

Netshroud opened this issue Oct 18, 2015 · 3 comments

Comments

@Netshroud
Copy link

Proof of concept:

        class JobFailureHandler : ClientMsgHandler
        {
            public override void HandleMsg(IPacketMsg packetMsg)
            {
                if (packetMsg.MsgType != EMsg.DestJobFailed)
                {
                    return;
                }

                var failure = new ClientMsg<MsgClientJustStrings>(packetMsg);
                var callback = new DestinationJobFailedCallback(failure.TargetJobID);
                steamClient.PostCallback(callback);

                Console.WriteLine("Job failed!");
            }
        }

        public class DestinationJobFailedCallback : CallbackMsg
        {
            public DestinationJobFailedCallback(JobID jobID)
            {
                this.JobID = jobID;
            }
        }

Only using MsgClientJustStrings as a placeholder. Actual response seems to have a 1-byte body, so far I've only seen it set to 0x00.

Not sure how this would work with #170.

@voided
Copy link
Member

voided commented Oct 19, 2015

So the behavior I'm seeing in steamclient regarding DestJobFailed is in CJob::BYieldingWaitForMsg. Here's a rough decompilation:

bool CJob::BYieldingWaitForMsg( IMsgNetPacket **ppNetPacket )
{
  JobMsgInfo_t *pJobMsgInfo; // [sp+18h] [bp-10h]@5

  if ( !this->m_szYieldReason )
  {
    SetYieldInfo( "Wait for msg", NULL );
  }

  if ( GetJobMgr().BYieldingWaitForMsg( this, ppNetPacket, &pJobMsgInfo ) )
  {
    while ( pJobMsgInfo->m_EMsg == k_EMsgJobHeartbeat )
    {
      m_STimeNextHeartbeat.SetFromServerTime( k_cMicroSecJobHeartbeat );
      SetYieldInfo( "Wait for msg", NULL );

      if ( !GetJobMgr().BYieldingWaitForMsg( this, ppNetPacket, &pJobMsgInfo) )
        return false;
    }

    AddPacketToList( ppNetPacket, pJobMsgInfo->m_JobIDSource );

    return pJobMsgInfo->m_EMsg != k_EMsgDestJobFailed;
  }

  return false;
}

Essentially, JobHeartbeat will keep a job alive on the client (by extending its timeout), and the final success of the job comes down to the EMsg the server sends down. The body of the message isn't inspected for heartbeats or failures.

This doesn't really translate well to callbacks, but could probably be handled gracefully with the existing work in #170.

@Netshroud
Copy link
Author

Oh, so that's where it is.

Some Jobs also use this one (Hopper pseudocode here), which seems more appropriate for our usage:

int CJob::BYieldingWaitForMsg(IProtoBufMsg*, EMsg)(void * arg_0, void arg_4) {
    var_10 = *___stack_chk_guard;
    eax = GetMessageList();
    CMessageList::GetMessage(eax, arg_8, var_214, 0x3b);
    CJob::SetYieldInfo(arg_0, var_214);
    if (CJob::BYieldingWaitForMsg(arg_0) != 0x0) {
            ebx = arg_4;
            eax = *ebx;
            (*(eax + 0x28))(ebx, 0x0);
            eax = *ebx;
            eax = (*(eax + 0x2c))(ebx);
            ecx = 0x1;
            esi = ___stack_chk_guard;
            if (eax != arg_8) {
                    eax = *ebx;
                    eax = (*(eax + 0x2c))(ebx);
                    CDbgFmtMsg::CDbgFmtMsg(var_210, "CJob::BYieldingWaitForMsg expected msg %u but received %u", arg_8, eax);
                    CDbgFmtMsg::CDbgFmtMsg(var_110, "Assertion Failed: %s", var_210);
                    AssertMsgImplementation(var_110, 0x0, "/Users/buildbot/buildslave/steam_rel_client_osx/build/src/common/job.cpp", 0x28e, 0x0);
                    ecx = 0x0;
            }
    }
    else {
            ecx = 0x0;
            esi = ___stack_chk_guard;
    }
    if (*esi == var_10) {
            eax = ecx & 0xff;
    }
    else {
            eax = __stack_chk_fail();
    }
    return eax;
}

@yaakov-h yaakov-h added this to the 2.1 milestone Sep 3, 2017
@yaakov-h
Copy link
Member

yaakov-h commented Sep 3, 2017

This is now handled for async jobs, but not for callback subscriptions.

I'm not sure what's the best way to handle this. The best I can think of is to use the same pattern used by Steamworks, where you can subscribe to a callback with Action<TCallback callback, bool didItFail>.

The other option I can think of is to get consumers to use async jobs instead of callbacks.

@xPaw xPaw modified the milestones: Future, Some day (PRs welcome) Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants