Implement complete HTTPS-RR support for AliasMode and ServiceMode records [chromium/src : main]
Eric Orth (Gerrit)
Eric Orth added 1 comment![]( "Open in Gerrit")
reviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Related details
- Helmut Januschka
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
reviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Yes, totally agreed with Eric's thoughts. We wouldn't be able to support aliases and multiple targets (service mode) at this point. There are several projects that have high priorities and conflicts with this CL. HEv3 is the main blocker, but in addition to HEv3, we also started working on querying HTTPS using the platform APIs ([1], [2]). We will need to refactor DnsTask and related classes so that we can share the same logic for both the built-in resolver and platform resolvers. That refactoring will conflict with this CL.
I didn't take a closer look at this CL (so I'm not very confident about the change). It seems this may work for happy paths, but as Eric pointed out, it's important to have robust logic to handle timeouts and cancellation so I expect that more work is needed to make it production ready.
Also wanted to mention that just querying followup queries doesn't fully support multiple target names (service mode). We also need to modify the connection attempt layer (HttpStreamFactory / HttpStreamPool) so that it can attempt connections even when the target name is different.
[1] https://crbug.com/448981097
[2] https://crbug.com/449966580
Related details
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Kenichi Ishibashireviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Yes, totally agreed with Eric's thoughts. We wouldn't be able to support aliases and multiple targets (service mode) at this point. There are several projects that have high priorities and conflicts with this CL. HEv3 is the main blocker, but in addition to HEv3, we also started working on querying HTTPS using the platform APIs ([1], [2]). We will need to refactor DnsTask and related classes so that we can share the same logic for both the built-in resolver and platform resolvers. That refactoring will conflict with this CL.
I didn't take a closer look at this CL (so I'm not very confident about the change). It seems this may work for happy paths, but as Eric pointed out, it's important to have robust logic to handle timeouts and cancellation so I expect that more work is needed to make it production ready.
Also wanted to mention that just querying followup queries doesn't fully support multiple target names (service mode). We also need to modify the connection attempt layer (HttpStreamFactory / HttpStreamPool) so that it can attempt connections even when the target name is different.
[1] https://crbug.com/448981097
[2] https://crbug.com/449966580
Thank you both for the detailed review and for taking the time to explain the broader context, really appreciate it!
Completely understand the timing concerns with the ongoing HEv3 work and platform API integration.
My question: Would there be value in keeping this CL as a reference implementation for post-HEv3 work, or would it be better to abandon it to avoid confusion?
I'm happy to:
- Help with HEv3 or platform API work if there's anything I can contribute
- Wait until HEv3 lands and then revisit with proper integration
- Abandon this entirely if the approach doesn't align with the architecture direction
What would you recommend?
Related details
- Eric Orth
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
Kenichi Ishibashireviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Helmut JanuschkaYes, totally agreed with Eric's thoughts. We wouldn't be able to support aliases and multiple targets (service mode) at this point. There are several projects that have high priorities and conflicts with this CL. HEv3 is the main blocker, but in addition to HEv3, we also started working on querying HTTPS using the platform APIs ([1], [2]). We will need to refactor DnsTask and related classes so that we can share the same logic for both the built-in resolver and platform resolvers. That refactoring will conflict with this CL.
I didn't take a closer look at this CL (so I'm not very confident about the change). It seems this may work for happy paths, but as Eric pointed out, it's important to have robust logic to handle timeouts and cancellation so I expect that more work is needed to make it production ready.
Also wanted to mention that just querying followup queries doesn't fully support multiple target names (service mode). We also need to modify the connection attempt layer (HttpStreamFactory / HttpStreamPool) so that it can attempt connections even when the target name is different.
[1] https://crbug.com/448981097
[2] https://crbug.com/449966580
Thank you both for the detailed review and for taking the time to explain the broader context, really appreciate it!
Completely understand the timing concerns with the ongoing HEv3 work and platform API integration.
My question: Would there be value in keeping this CL as a reference implementation for post-HEv3 work, or would it be better to abandon it to avoid confusion?
I'm happy to:
- Help with HEv3 or platform API work if there's anything I can contribute
- Wait until HEv3 lands and then revisit with proper integration
- Abandon this entirely if the approach doesn't align with the architecture directionWhat would you recommend?
My preference would be "Wait until HEv3 lands and then revisit with proper integration." At this point I don't have concrete ideas about how we should implement aliases and different target names so I'm not sure we keep or abandon this CL. I think marking this as work in progress (via "more action" three-dots) is a reasonable action for now.
Related details
- Eric Orth
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Kenichi Ishibashireviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Helmut JanuschkaYes, totally agreed with Eric's thoughts. We wouldn't be able to support aliases and multiple targets (service mode) at this point. There are several projects that have high priorities and conflicts with this CL. HEv3 is the main blocker, but in addition to HEv3, we also started working on querying HTTPS using the platform APIs ([1], [2]). We will need to refactor DnsTask and related classes so that we can share the same logic for both the built-in resolver and platform resolvers. That refactoring will conflict with this CL.
I didn't take a closer look at this CL (so I'm not very confident about the change). It seems this may work for happy paths, but as Eric pointed out, it's important to have robust logic to handle timeouts and cancellation so I expect that more work is needed to make it production ready.
Also wanted to mention that just querying followup queries doesn't fully support multiple target names (service mode). We also need to modify the connection attempt layer (HttpStreamFactory / HttpStreamPool) so that it can attempt connections even when the target name is different.
[1] https://crbug.com/448981097
[2] https://crbug.com/449966580
Kenichi IshibashiThank you both for the detailed review and for taking the time to explain the broader context, really appreciate it!
Completely understand the timing concerns with the ongoing HEv3 work and platform API integration.
My question: Would there be value in keeping this CL as a reference implementation for post-HEv3 work, or would it be better to abandon it to avoid confusion?
I'm happy to:
- Help with HEv3 or platform API work if there's anything I can contribute
- Wait until HEv3 lands and then revisit with proper integration
- Abandon this entirely if the approach doesn't align with the architecture directionWhat would you recommend?
My preference would be "Wait until HEv3 lands and then revisit with proper integration." At this point I don't have concrete ideas about how we should implement aliases and different target names so I'm not sure we keep or abandon this CL. I think marking this as work in progress (via "more action" three-dots) is a reasonable action for now.
thanks, will put it on WIP for now, updated timeout and abort anyway
Related details
- Eric Orth
- Kenichi Ishibashi
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
Kenichi Ishibashireviewers + bashi@ to get another look at this
I'll try to take a more detailed look at this later this week, just starting with a quick big-picture review...
I think this work will definitely need to be feature guarded with a default-disabled feature per https://chromium.googlesource.com/chromium/src/+/main/docs/flag_guarding_guidelines.md to allow for a careful rollout.
But as written, I strongly suspect we wouldn't be able to launch enabling this functionality without first waiting for full completion of Happy Eyeballs v3 (HEv3) support in Chrome. This CL implements the followup queries mostly at the HostResolverManager::Job level, bypassing some important timing control logic in HostResolverDnsTask that cancels out of waiting for HTTPS results if taking too long after completion of A/AAAA results, potentially delaying completion of a ResolveHostRequest until after all followup queries are complete, potentially well after completion of simple A/AAAA queries. When we've tried relaxing that timing control in the past, e.g. to just always wait for HTTPS results, we've found that it unacceptably slows down Chrome performance, and our general conclusion has been that we need HEv3 to allow intelligently moving forward with connection logic based on intermediate results passed through DnsTaskResultsManager.
Helmut JanuschkaYes, totally agreed with Eric's thoughts. We wouldn't be able to support aliases and multiple targets (service mode) at this point. There are several projects that have high priorities and conflicts with this CL. HEv3 is the main blocker, but in addition to HEv3, we also started working on querying HTTPS using the platform APIs ([1], [2]). We will need to refactor DnsTask and related classes so that we can share the same logic for both the built-in resolver and platform resolvers. That refactoring will conflict with this CL.
I didn't take a closer look at this CL (so I'm not very confident about the change). It seems this may work for happy paths, but as Eric pointed out, it's important to have robust logic to handle timeouts and cancellation so I expect that more work is needed to make it production ready.
Also wanted to mention that just querying followup queries doesn't fully support multiple target names (service mode). We also need to modify the connection attempt layer (HttpStreamFactory / HttpStreamPool) so that it can attempt connections even when the target name is different.
[1] https://crbug.com/448981097
[2] https://crbug.com/449966580
Kenichi IshibashiThank you both for the detailed review and for taking the time to explain the broader context, really appreciate it!
Completely understand the timing concerns with the ongoing HEv3 work and platform API integration.
My question: Would there be value in keeping this CL as a reference implementation for post-HEv3 work, or would it be better to abandon it to avoid confusion?
I'm happy to:
- Help with HEv3 or platform API work if there's anything I can contribute
- Wait until HEv3 lands and then revisit with proper integration
- Abandon this entirely if the approach doesn't align with the architecture directionWhat would you recommend?
Helmut JanuschkaMy preference would be "Wait until HEv3 lands and then revisit with proper integration." At this point I don't have concrete ideas about how we should implement aliases and different target names so I'm not sure we keep or abandon this CL. I think marking this as work in progress (via "more action" three-dots) is a reasonable action for now.
thanks, will put it on WIP for now, updated timeout and abort anyway
you have any ideas when this will happen? or any path forward?
Related details
- Eric Orth
- Kenichi Ishibashi
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. |
Kenichi Ishibashi (Gerrit)
Kenichi Ishibashi added 1 comment![]( "Open in Gerrit")
Th HEv3 development has be prolonged and it's difficult to predict when it will be completed. The team are moving towards not implementing HEv3 (that's unfortunate).
We may consider supporting HTTPS RR aliases and different target names without HEv3, nothing has been decided yet. We are currently discussing what to do.
Related details
- Eric Orth
- Helmut Januschka
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. |
Helmut Januschka (Gerrit)
Helmut Januschka abandoned this change.
Helmut Januschka abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |