Forwarding abort reason from AbortController.abort() to fetch() response [chromium/src : main]
Taylor Brandstetter (Gerrit)
Taylor Brandstetter added 1 comment![]( "Open in Gerrit")
Oops, thought I was adding reviewers earlier but was only adding to CC. See previous message.
Related details
- Adam Rice
- Andreas Haas
- David Bokan
- Hiroki Nakagawa
- Rakina Zata Amni
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rakina Zata Amni (Gerrit)
Rakina Zata Amni added 1 comment![]( "Open in Gerrit")
(I think the other reviewers already cover the ownership needed for review, so let me remove myself)
Related details
- Adam Rice
- Andreas Haas
- David Bokan
- Hiroki Nakagawa
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan added 2 comments![]( "Open in Gerrit")
David BokanI'm doing this following a discussion with Adam. The context is that we (Meet developers) would like the stack information that CreateOrEmpty provides, so we were thinking of just changing body.cc to use that, and preferably adding the fetch URL in the message as well for additional debuggability.
That would require an experiment for changing the message. But Adam also noticed that we should be providing the abort reason as well (if one is provided), which also would require an experiment. In which case we might as well do both at once.
A question though: What would be the best way to go about getting the URL into this message, assuming that's not problematic? The error is now coming from AbortSignal (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/abort_signal.cc;l=279;drc=0a4317210c4de343411da25ccf8145d3091b3007) which doesn't have that context. Could I provide an optional string to its constructor for a default abort reason? Or would the memory footprint for that be a concern?
Apologies for the delay. I can't say I have a ton of expertise specifically in Fetch/Abort as a general Blink core dev but given this isn't long lived and URLs aren't that big I don't see the issue in copying in the KURL. You could even pass in and hold a pointer to `FetchRequestData` which is GarbageCollected, assuming all this makes sense for Abort (which Adam would be better placed to judge)
Removing myself since I don't think I'm the best reviewer in the list for fetch-related things.
Related details
- Adam Rice
- Andreas Haas
- Hiroki Nakagawa
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice voted and added 2 comments![]( "Open in Gerrit")
Votes added by Adam Rice
| Code-Review | +1 |
2 comments
lgtm. Sorry for the delay. Nice clean CL.
while (true) {
// Stop spinning if 10 seconds have passed
if (Date.now() - start > 10000) throw Error('Timed out');
const afterAbortResult = await fetch(`../resources/stash-take.py?key=${stateKey}`).then(r => r.json());
if (afterAbortResult == 'closed') break;
}testharness.js provides a cleaner way to do this:
```suggestion
await t.step_wait(async () => {
const response = await fetch(`../resources/stash-take.py?key=${stateKey}`);
const json = await response.json();
return json == 'closed';
}, "underlying connection should close");
```
Related details
Taylor Brandstetter (Gerrit)
Taylor Brandstetter added 2 comments![]( "Open in Gerrit")
David BokanI'm doing this following a discussion with Adam. The context is that we (Meet developers) would like the stack information that CreateOrEmpty provides, so we were thinking of just changing body.cc to use that, and preferably adding the fetch URL in the message as well for additional debuggability.
That would require an experiment for changing the message. But Adam also noticed that we should be providing the abort reason as well (if one is provided), which also would require an experiment. In which case we might as well do both at once.
A question though: What would be the best way to go about getting the URL into this message, assuming that's not problematic? The error is now coming from AbortSignal (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/abort_signal.cc;l=279;drc=0a4317210c4de343411da25ccf8145d3091b3007) which doesn't have that context. Could I provide an optional string to its constructor for a default abort reason? Or would the memory footprint for that be a concern?
Apologies for the delay. I can't say I have a ton of expertise specifically in Fetch/Abort as a general Blink core dev but given this isn't long lived and URLs aren't that big I don't see the issue in copying in the KURL. You could even pass in and hold a pointer to `FetchRequestData` which is GarbageCollected, assuming all this makes sense for Abort (which Adam would be better placed to judge)
Realized some issues:
- The latter approach would introduce a dependency on core/fetch from core/dom which I assume we don't want
- More fundamentally, you can use the same abort signal for multiple fetches...
So I really can't think of any way to do this besides replacing the default error at the fetch level. Which seems a little ugly. Should I just give up on this or any other suggestions?
while (true) {
// Stop spinning if 10 seconds have passed
if (Date.now() - start > 10000) throw Error('Timed out');
const afterAbortResult = await fetch(`../resources/stash-take.py?key=${stateKey}`).then(r => r.json());
if (afterAbortResult == 'closed') break;
}testharness.js provides a cleaner way to do this:
```suggestion
await t.step_wait(async () => {
const response = await fetch(`../resources/stash-take.py?key=${stateKey}`);
const json = await response.json();
return json == 'closed';
}, "underlying connection should close");
```
- Adam Rice
- Andreas Haas
- David Bokan
- Hiroki Nakagawa
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Adam Rice (Gerrit)
Adam Rice added 1 comment![]( "Open in Gerrit")
David BokanI'm doing this following a discussion with Adam. The context is that we (Meet developers) would like the stack information that CreateOrEmpty provides, so we were thinking of just changing body.cc to use that, and preferably adding the fetch URL in the message as well for additional debuggability.
That would require an experiment for changing the message. But Adam also noticed that we should be providing the abort reason as well (if one is provided), which also would require an experiment. In which case we might as well do both at once.
A question though: What would be the best way to go about getting the URL into this message, assuming that's not problematic? The error is now coming from AbortSignal (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/abort_signal.cc;l=279;drc=0a4317210c4de343411da25ccf8145d3091b3007) which doesn't have that context. Could I provide an optional string to its constructor for a default abort reason? Or would the memory footprint for that be a concern?
Taylor BrandstetterApologies for the delay. I can't say I have a ton of expertise specifically in Fetch/Abort as a general Blink core dev but given this isn't long lived and URLs aren't that big I don't see the issue in copying in the KURL. You could even pass in and hold a pointer to `FetchRequestData` which is GarbageCollected, assuming all this makes sense for Abort (which Adam would be better placed to judge)
Realized some issues:
- The latter approach would introduce a dependency on core/fetch from core/dom which I assume we don't want
- More fundamentally, you can use the same abort signal for multiple fetches...
So I really can't think of any way to do this besides replacing the default error at the fetch level. Which seems a little ugly. Should I just give up on this or any other suggestions?
I think if a message wasn't passed to abort(), Fetch can justifiably override the default message. This would mean that AbortSignal would need some kind of flag saying "I still have my default AbortError exception" for Fetch to look at, and also an internal API for Fetch to replace the exception. Then it can include the URL in the message if it is same-origin with the page.
If multiple fetches have been passed the same AbortSignal, the first one to replace the exception wins.
I don't know whether it is possible to correctly preserve the stack if this is done.
It's probably worth doing in a separate CL anyway.
Related details
- Andreas Haas
- Hiroki Nakagawa
- Taylor Brandstetter
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taylor Brandstetter (Gerrit)
Taylor Brandstetter added 1 comment![]( "Open in Gerrit")
David BokanI'm doing this following a discussion with Adam. The context is that we (Meet developers) would like the stack information that CreateOrEmpty provides, so we were thinking of just changing body.cc to use that, and preferably adding the fetch URL in the message as well for additional debuggability.
That would require an experiment for changing the message. But Adam also noticed that we should be providing the abort reason as well (if one is provided), which also would require an experiment. In which case we might as well do both at once.
A question though: What would be the best way to go about getting the URL into this message, assuming that's not problematic? The error is now coming from AbortSignal (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/abort_signal.cc;l=279;drc=0a4317210c4de343411da25ccf8145d3091b3007) which doesn't have that context. Could I provide an optional string to its constructor for a default abort reason? Or would the memory footprint for that be a concern?
Taylor BrandstetterApologies for the delay. I can't say I have a ton of expertise specifically in Fetch/Abort as a general Blink core dev but given this isn't long lived and URLs aren't that big I don't see the issue in copying in the KURL. You could even pass in and hold a pointer to `FetchRequestData` which is GarbageCollected, assuming all this makes sense for Abort (which Adam would be better placed to judge)
Adam RiceRealized some issues:
- The latter approach would introduce a dependency on core/fetch from core/dom which I assume we don't want
- More fundamentally, you can use the same abort signal for multiple fetches...
So I really can't think of any way to do this besides replacing the default error at the fetch level. Which seems a little ugly. Should I just give up on this or any other suggestions?
I think if a message wasn't passed to abort(), Fetch can justifiably override the default message. This would mean that AbortSignal would need some kind of flag saying "I still have my default AbortError exception" for Fetch to look at, and also an internal API for Fetch to replace the exception. Then it can include the URL in the message if it is same-origin with the page.
If multiple fetches have been passed the same AbortSignal, the first one to replace the exception wins.
I don't know whether it is possible to correctly preserve the stack if this is done.
It's probably worth doing in a separate CL anyway.
Alright I'll just land this as-is, I guess as long as the flag isn't rolled out those time to change things
Related details
- Adam Rice
- Andreas Haas
- David Bokan
- Hiroki Nakagawa
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Taylor Brandstetter (Gerrit)
Taylor Brandstetter added 1 comment![]( "Open in Gerrit")
Sorry, need updated approvals, didn't realize rebasing would do that.
Related details
- Adam Rice
- Andreas Haas
- David Bokan
- Hiroki Nakagawa
- Taylor Brandstetter
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |