1151414 - RDP client requests should set async caller

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

RDP client requests should use |callFunctionWithAsyncStack| to connect the code making the request with the eventual processing of the server's reply.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that this only makes sense when debugging locally with non-e10s builds. The async stack machinery can't cross processes.

We could just stringify and attach the stack to packets directly.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> Note that this only makes sense when debugging locally with non-e10s builds.
> The async stack machinery can't cross processes.
> 
> We could just stringify and attach the stack to packets directly.

I think you're imagining a different variant than I was thinking of originally (though it also sounds useful!).  Let's take a look at the request steps:

1. Tool running on client makes request
2. Client sends request packet to server
3. Server receives request
4. Server does work to process request
5. Server sends reply to client
6. Client receives reply from server
7. Client does work to process reply

Currently, we get disconnected stacks for 1-2, 3-5, and 6-7.  I would like to 1-2 linked to the reply processing in 6-7, so that instead of just seeing an error processing a reply, you can also see how the request was initiated.  Since this work only happens on the client in all cases, it seems it should be doable for any connection type.

As you say though, for local non-e10s, we could link the entire chain since the server is the same process.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #33)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32)
> > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30)
> > > Ugh... Can we disallow JS to explicitly control this? That seems nasty... Or
> > > maybe just disallow JS from providing its own asyncCause and have it be the
> > > generic "Async" string?
> > 
> > Hmm, I believe I was planning to use this from JS to connect things like
> > local DevTools RDP stacks (bug 1151414).
> > 
> > Wouldn't I need this method to make the async stack link from JS, or do I
> > misunderstand how this works?
> 
> I don't think that the async stacks infrastructure is suited for tracing RDP
> messages because they are cross process (or even cross device). I think
> optionally adding a "_stack: Error().stack" property to each packet would
> suffice, but let's move this conversation to that bug.

As I mention in comment 2, I am mostly interested in unify the **client's** view of what happens to an RDP request, so there are no cross process or cross device concerns.

It seems just the same as a promise or other async utility to me.  Why are async stacks not what we want here?  It sounds like you are suggesting I manually recreate the behavior, but I don't follow why.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #33)
> > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32)
> > > (In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #30)
> > > > Ugh... Can we disallow JS to explicitly control this? That seems nasty... Or
> > > > maybe just disallow JS from providing its own asyncCause and have it be the
> > > > generic "Async" string?
> > > 
> > > Hmm, I believe I was planning to use this from JS to connect things like
> > > local DevTools RDP stacks (bug 1151414).
> > > 
> > > Wouldn't I need this method to make the async stack link from JS, or do I
> > > misunderstand how this works?
> > 
> > I don't think that the async stacks infrastructure is suited for tracing RDP
> > messages because they are cross process (or even cross device). I think
> > optionally adding a "_stack: Error().stack" property to each packet would
> > suffice, but let's move this conversation to that bug.
> 
> As I mention in comment 2, I am mostly interested in unify the **client's**
> view of what happens to an RDP request, so there are no cross process or
> cross device concerns.
> 
> It seems just the same as a promise or other async utility to me.  Why are
> async stacks not what we want here?  It sounds like you are suggesting I
> manually recreate the behavior, but I don't follow why.

Ah sorry, I forgot that we went through this already, and you clarified that you don't want server<-->client stacks, but client<-->client.

Yeah then you can reuse the async stacks infrastructure. We could still remove the custom asyncCause string, and I think that would be a fine compromise.

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Nick Fitzgerald [:fitzgen][:nf] from comment #4)
> Yeah then you can reuse the async stacks infrastructure. We could still
> remove the custom asyncCause string, and I think that would be a fine
> compromise.

Ah okay, then that sounds fine to me.  Linking the stacks from JS is all I really care about, the |asyncCause| field is not critical.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66286ac738f6

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Missed part of the commit last time...

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5d6dfca7d

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: 0001-Bug-1151414-Link-stacks-across-RDP-client-request-an.patch

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8626467 [details] [diff] [review]
0001-Bug-1151414-Link-stacks-across-RDP-client-request-an.patch

Review of attachment 8626467 [details] [diff] [review]:
-----------------------------------------------------------------

That was a lot more straightforward than I expected, which is great. Thanks for doing this!

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5b03881a38de

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5b03881a38de