Closed Bug 1151414 Opened 5 years ago Closed 4 years ago

RDP client requests should set async caller

Categories

(DevTools :: Framework, defect)

defect
Not set

Tracking

(firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

RDP client requests should use |callFunctionWithAsyncStack| to connect the code making the request with the eventual processing of the server's reply.
No longer depends on: 1151413
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.
(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.
(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.
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
(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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Missed part of the commit last time...

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5d6dfca7d
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!
Attachment #8626467 - Flags: review?(nfitzgerald) → review+
Keywords: checkin-needed
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5b03881a38de
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.