Closed
Bug 1151414
Opened 7 years ago
Closed 7 years ago
RDP client requests should set async caller
Categories
(DevTools :: Framework, defect)
DevTools
Framework
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)
10.55 KB,
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
RDP client requests should use |callFunctionWithAsyncStack| to connect the code making the request with the eventual processing of the server's reply.
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
(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.
Assignee | ||
Comment 3•7 years ago
|
||
(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)
Comment 4•7 years ago
|
||
(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)
Assignee | ||
Comment 5•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=66286ac738f6
Assignee | ||
Comment 7•7 years ago
|
||
Missed part of the commit last time... Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5d6dfca7d
Assignee | ||
Comment 8•7 years ago
|
||
Attachment #8626467 -
Flags: review?(nfitzgerald)
Comment 9•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 11•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b03881a38de
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Updated•4 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•