Initial TcpConnectJob implementation. [chromium/src : main]
mmenke (Gerrit)
mmenke added 2 comments![]( "Open in Gerrit")
[bashi] Thoughts? Feel free to take your time. I may send out small CLs on top of this later today/this week, but don't feel like that means there's a rush on this one. I renamed SubJob to Connector, because talking about "jobs" and "sub jobs" was just too confusing, though I may still have slipped up in some places.
The plan is to address the small todos (connect timing, connect attempts, the host resolution callback invocation, etc) in fairly small followups, then do the big ones (second connector), net log (probably not huge, but best done after the second connector is in place), and wiring up to callers (with some integration tests).
if (parent_job_->websocket_endpoint_lock_manager()) {Note that this is on top of a CL that I sent to ricea@ that simplifies the WebSocket code needed in this CL. It's possible things will change as that review progresses, but I'll just keep this completely in sync with whatever we do there.
I don't intend to add any unit tests for this call. There are tests at the websocket layer, which pass when the feature is enabled. I may make them run with and without the feature when I actually wire things up, but am not too concerned about this.
Related details
- 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 9 comments![]( "Open in Gerrit")
Sorry I wasn't able to take an actual look. Just leaving minor comments. I'll take a look tomorrow.
std::unique_ptr<TcpConnectJob::Connector> connector_;nit, optional: We can omit `TcpConnectJob::` here.
void IfCompletedNotifyDelegate(int result);To me, `NotifyDelegateIfCompleted` is more readable. What do you think?
using IPEndPointInfo = base::expected<IPEndPoint, Error>;(Just leaving comments for possible future changes)
When we support HTTPS RR follow-up queries (alias and different target names), each "endpoint" may have different alpns and/or different SvcParams (e.g. ECH config). In the future, we may want to track attempted/attempting endpoint including these information, not just IPEndPoint.
// IPEndPoints are provided to child jobs with this API.connector?
static constexpr base::TimeDelta kIPv6FallbackTime = base::Milliseconds(300);I guess you intentionally duplicate this and other stuff (e.g. ServiceEndpointOverride) from TransportConnectJob, assuming we will remove TransportConnectJob eventually.
// HandleServiceEndpointsUpdated(), and HandleAdvanceWaitingJobs(). Each ofMaybe `Connector`? Ditto for other "job"s that refer to `Connector`.
// Note that there is no start method. Instead, Connectorss are started with aConnectors
// Note that there is no start method. Instead, Connectorss are started with aProbably better to put this design choice in the class documentation above?
Also it would be helpful to mention that when the parent job creates an instance of Connector, after we add the second connector in subsequent CLs.
Related details
- 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. |
mmenke (Gerrit)
mmenke added 9 comments![]( "Open in Gerrit")
Sorry I wasn't able to take an actual look. Just leaving minor comments. I'll take a look tomorrow.
No worries. It's a big CL, and there's no urgency here. Rather get it done well than fast. And just your early comments were really useful - I had thought this was in better shape in terms of comments than it was, so I've spent some time today going over it today, and looking for ways to improve my comments, as well as done some other minor refactors.
Thanks, Kenichi!
void IfCompletedNotifyDelegate(int result);To me, `NotifyDelegateIfCompleted` is more readable. What do you think?
"NotifyDelegateIfCompleted " looks too much like "NotifyDelegateOfCompletion" for my tastes. That having been said, there are other options. I've renamed to NotifyDelegateIfDone(). Open to other options.
using IPEndPointInfo = base::expected<IPEndPoint, Error>;(Just leaving comments for possible future changes)
When we support HTTPS RR follow-up queries (alias and different target names), each "endpoint" may have different alpns and/or different SvcParams (e.g. ECH config). In the future, we may want to track attempted/attempting endpoint including these information, not just IPEndPoint.
I assume we'd still want to connect before crypto ready, so we'd still be passing around raw IPEndpoints, though that does potentially lead to a new weird case: We have connections to non-crypto-ready IPs, but other IPs are crypto ready. What do we do?
It may actually make sense to make a 3rd connection in that case, while keeping old connections around I guess?
// IPEndPoints are provided to child jobs with this API.mmenkeconnector?
Done
static constexpr base::TimeDelta kIPv6FallbackTime = base::Milliseconds(300);I guess you intentionally duplicate this and other stuff (e.g. ServiceEndpointOverride) from TransportConnectJob, assuming we will remove TransportConnectJob eventually.
TransportConnectJob doesn't have ServiceEndpointOverride - it has EndpointResultOverride, which wraps HostResolverEndpointResult instead of ServiceEndpoint. Using a variant for that didn't seem useful.
But, yea, I duplicated the time stuff because there's not a lot of it, and I imagine us removing the old code. Also, it's entirely possible optimal times would be different for this class than the other one.
// HandleServiceEndpointsUpdated(), and HandleAdvanceWaitingJobs(). Each ofMaybe `Connector`? Ditto for other "job"s that refer to `Connector`.
Done. I've audited all "jobs", and replaced all of them with "[Tcp]ConnectJob" or "Connector". Sorry about that, thought I did a better job of getting rid of them yesterday.
// Note that there is no start method. Instead, Connectorss are started with ammenkeConnectors
Done
// Note that there is no start method. Instead, Connectorss are started with aProbably better to put this design choice in the class documentation above?
Also it would be helpful to mention that when the parent job creates an instance of Connector, after we add the second connector in subsequent CLs.
Done
Related details
- 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. |
mmenke (Gerrit)
mmenke added 1 comment![]( "Open in Gerrit")
using IPEndPointInfo = base::expected<IPEndPoint, Error>;mmenke(Just leaving comments for possible future changes)
When we support HTTPS RR follow-up queries (alias and different target names), each "endpoint" may have different alpns and/or different SvcParams (e.g. ECH config). In the future, we may want to track attempted/attempting endpoint including these information, not just IPEndPoint.
I assume we'd still want to connect before crypto ready, so we'd still be passing around raw IPEndpoints, though that does potentially lead to a new weird case: We have connections to non-crypto-ready IPs, but other IPs are crypto ready. What do we do?
It may actually make sense to make a 3rd connection in that case, while keeping old connections around I guess?
So, right now, what this class needs crypto ready for is to know two things (I think):
1) We know the final value of `svcb_optional_`. This is the part I'm most familiar with, since it's dealt with directly by this ConnectJob, so it's what I've been associating with crypto ready.
2) We know when we have enough information to let the SSL layer connect. This is actually what crypto ready is named after.
If crypto ready information trickles in, when would we know enough to set `svcb_optional_`? I assume it would still have one value for the entire request, but not clear to me if we'd get it early or late. If it's late, we could presumably still complete before we have it, if we know an endpoint is crypto ready (and thus have a SVCB record for it).
Related details
- 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 voted and added 15 comments![]( "Open in Gerrit")
Votes added by Kenichi Ishibashi
| Code-Review | +1 |
15 comments
Left several comments, but none of them are blocking, so CR+1.
I've checked most tests, but I may miss something. Will try to check them as follow-ups.
Thanks,
const ServiceEndpoint* FindServiceEndpoint(std::optional<ServiceEndpoint> since we make a copy if we find available one?
using IPEndPointInfo = base::expected<IPEndPoint, Error>;mmenke(Just leaving comments for possible future changes)
When we support HTTPS RR follow-up queries (alias and different target names), each "endpoint" may have different alpns and/or different SvcParams (e.g. ECH config). In the future, we may want to track attempted/attempting endpoint including these information, not just IPEndPoint.
mmenkeI assume we'd still want to connect before crypto ready, so we'd still be passing around raw IPEndpoints, though that does potentially lead to a new weird case: We have connections to non-crypto-ready IPs, but other IPs are crypto ready. What do we do?
It may actually make sense to make a 3rd connection in that case, while keeping old connections around I guess?
So, right now, what this class needs crypto ready for is to know two things (I think):
1) We know the final value of `svcb_optional_`. This is the part I'm most familiar with, since it's dealt with directly by this ConnectJob, so it's what I've been associating with crypto ready.
2) We know when we have enough information to let the SSL layer connect. This is actually what crypto ready is named after.If crypto ready information trickles in, when would we know enough to set `svcb_optional_`? I assume it would still have one value for the entire request, but not clear to me if we'd get it early or late. If it's late, we could presumably still complete before we have it, if we know an endpoint is crypto ready (and thus have a SVCB record for it).
To calculate `svcb_optional_` for different endpoints, we probably need to modify the ServiceEndpointRequest API first so that we can distinguish which endpoints are crypto ready and which endpoints are not crypto ready.
I haven't really thought about what kind of interface would be best, maybe something like:
```
class ServiceEndpointRequest {
bool IsEndpointsCryptoReady(const ServiceEndpoint& endpoint) = 0;
};
```
or
```
class ServiceEndpoint {
bool endpoint_crypto_ready = false;
};
```
Not entirely sure about this. Let me loop hel...@januschka.com in since they proposed HTTPS follow-up queries.
https://docs.google.com/document/d/1Xku49GjTNUccpuZtJnImWN90fJgX43d9_rH91vHs7wc/edit?usp=sharing
helmut@: Note that we haven't decided we would like you to work on that. Just sharing information.
static constexpr base::TimeDelta kIPv6FallbackTime = base::Milliseconds(300);mmenkeI guess you intentionally duplicate this and other stuff (e.g. ServiceEndpointOverride) from TransportConnectJob, assuming we will remove TransportConnectJob eventually.
TransportConnectJob doesn't have ServiceEndpointOverride - it has EndpointResultOverride, which wraps HostResolverEndpointResult instead of ServiceEndpoint. Using a variant for that didn't seem useful.
But, yea, I duplicated the time stuff because there's not a lot of it, and I imagine us removing the old code. Also, it's entirely possible optimal times would be different for this class than the other one.
Ah you're right TransportConnectJob has EndpointResultOverride but this has ServiceEndpointOverride.
Ack this is intended.
int last_error_ = ERR_NAME_NOT_RESOLVED;optional: How about using std::optional<int> and return ERR_NAME_NOT_RESOLVED in DoObtainIPEndPoint() when no more IP endpoints to attempt? Mostly for putting logic and comment into a single place.
if (parent_job_->websocket_endpoint_lock_manager()) {Note that this is on top of a CL that I sent to ricea@ that simplifies the WebSocket code needed in this CL. It's possible things will change as that review progresses, but I'll just keep this completely in sync with whatever we do there.
I don't intend to add any unit tests for this call. There are tests at the websocket layer, which pass when the feature is enabled. I may make them run with and without the feature when I actually wire things up, but am not too concerned about this.
Acknowledged
if (error == ERR_NETWORK_IO_SUSPENDED) {Oh I wasn't aware this logic (in the old TransportConnectJob). I probably want to add similar logic to HEv3, if we keep it.
builder.set_ech_config_list({'?'});Could you add a comment the `?` value can be anything, not that important.
// Use something othen than ERR_NAME_NOT_RESOLVED because that's the defaultother?
// Use something othen than ERR_NAME_NOT_RESOLVED because that's the defaultditt, other?
// IP address, which should be enough for the job to connect and return aShould we reset ServiceEndpointRequest and mark DNS resolution completed successfully for this case? Or will the Job be expected to being destroyed immediately after the consumer takes the result?
AddConnect(nit: Can we have a comment that the first endpoint fails.
int service_endpoint;optional: service_endpint_index? (This is a bit cryptic to me, but I don't have a good idea to make it more easy to understand)
// This will result in trying IPS in this order, since IPv4 and IPv6IPs ?
// This will result in trying IPS in this order, since IPv4 and IPv6Ditto, IPs ?
Related details
- mmenke
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. |
mmenke (Gerrit)
mmenke voted and added 13 comments![]( "Open in Gerrit")
Votes added by mmenke
| Commit-Queue | +2 |
13 comments
Thanks, Kenichi! I'm going to go ahead and land this, happy to address any followup comments in future CLs.
std::optional<ServiceEndpoint> since we make a copy if we find available one?
Not done. The method just below doesn't want a copy, it just wants to check if IPEndPoint is usable, which it does by checking if this returns a non-null value. We don't want/need to copy in that case.
using IPEndPointInfo = base::expected<IPEndPoint, Error>;mmenke(Just leaving comments for possible future changes)
When we support HTTPS RR follow-up queries (alias and different target names), each "endpoint" may have different alpns and/or different SvcParams (e.g. ECH config). In the future, we may want to track attempted/attempting endpoint including these information, not just IPEndPoint.
mmenkeI assume we'd still want to connect before crypto ready, so we'd still be passing around raw IPEndpoints, though that does potentially lead to a new weird case: We have connections to non-crypto-ready IPs, but other IPs are crypto ready. What do we do?
It may actually make sense to make a 3rd connection in that case, while keeping old connections around I guess?
Kenichi IshibashiSo, right now, what this class needs crypto ready for is to know two things (I think):
1) We know the final value of `svcb_optional_`. This is the part I'm most familiar with, since it's dealt with directly by this ConnectJob, so it's what I've been associating with crypto ready.
2) We know when we have enough information to let the SSL layer connect. This is actually what crypto ready is named after.If crypto ready information trickles in, when would we know enough to set `svcb_optional_`? I assume it would still have one value for the entire request, but not clear to me if we'd get it early or late. If it's late, we could presumably still complete before we have it, if we know an endpoint is crypto ready (and thus have a SVCB record for it).
To calculate `svcb_optional_` for different endpoints, we probably need to modify the ServiceEndpointRequest API first so that we can distinguish which endpoints are crypto ready and which endpoints are not crypto ready.
I haven't really thought about what kind of interface would be best, maybe something like:
```
class ServiceEndpointRequest {
bool IsEndpointsCryptoReady(const ServiceEndpoint& endpoint) = 0;
};
```or
```
class ServiceEndpoint {
bool endpoint_crypto_ready = false;
};
```Not entirely sure about this. Let me loop hel...@januschka.com in since they proposed HTTPS follow-up queries.
https://docs.google.com/document/d/1Xku49GjTNUccpuZtJnImWN90fJgX43d9_rH91vHs7wc/edit?usp=sharinghelmut@: Note that we haven't decided we would like you to work on that. Just sharing information.
Acknowledged
optional: How about using std::optional<int> and return ERR_NAME_NOT_RESOLVED in DoObtainIPEndPoint() when no more IP endpoints to attempt? Mostly for putting logic and comment into a single place.
Done
if (parent_job_->websocket_endpoint_lock_manager()) {Kenichi IshibashiNote that this is on top of a CL that I sent to ricea@ that simplifies the WebSocket code needed in this CL. It's possible things will change as that review progresses, but I'll just keep this completely in sync with whatever we do there.
I don't intend to add any unit tests for this call. There are tests at the websocket layer, which pass when the feature is enabled. I may make them run with and without the feature when I actually wire things up, but am not too concerned about this.
Acknowledged
I've removed the WebSocket code, since that CL hasn't landed yet. I'll wire it up in another CL (Added TODO)
Could you add a comment the `?` value can be anything, not that important.
Done
// Use something othen than ERR_NAME_NOT_RESOLVED because that's the defaultmmenkeother?
Done
// Use something othen than ERR_NAME_NOT_RESOLVED because that's the defaultmmenkeditt, other?
Done
// IP address, which should be enough for the job to connect and return aShould we reset ServiceEndpointRequest and mark DNS resolution completed successfully for this case? Or will the Job be expected to being destroyed immediately after the consumer takes the result?
If we destroy that request, we'll need to cache the ServiceEndpoint, and if we want to avoid copying it twice, GetServiceEndpoint() would need to become TakeServiceEndpoint(), and std::move() it. There's a TODO suggesting we do that.
Anyhow, the job not being deleted immediately in this case is the reason for the `is_done_` changes in the DNS callbacks. I think this is definitely something worth investigating further, but not in this CL.
nit: Can we have a comment that the first endpoint fails.
Done
optional: service_endpint_index? (This is a bit cryptic to me, but I don't have a good idea to make it more easy to understand)
Done (+comment)
// This will result in trying IPS in this order, since IPv4 and IPv6mmenkeIPs ?
Done
// This will result in trying IPS in this order, since IPv4 and IPv6mmenkeDitto, IPs ?
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: net/socket/tcp_connect_job.cc
Insertions: 5, Deletions: 3.
The diff is too large to show. Please review the diff.
```
```
The name of the file: net/socket/tcp_connect_job_connector.cc
Insertions: 6, Deletions: 14.
The diff is too large to show. Please review the diff.
```
```
The name of the file: net/socket/tcp_connect_job_unittest.cc
Insertions: 11, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: net/socket/tcp_connect_job.h
Insertions: 4, Deletions: 4.
The diff is too large to show. Please review the diff.
```
```
The name of the file: net/socket/tcp_connect_job_connector.h
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
Initial TcpConnectJob implementation.
This is the first CL for a relatively simple upgrade of the TCP
connection establishment logic to Happy Eyeballs v2 + HTTPS record
awareness.
The idea is to establish IPv4/v6 connections as the component DNS
requests complete, but to respect HTTPS records, and respect
ServiceEndpoint priority as best we can.
This CL only makes one connection at a time, alternating v4 and v6
addresses, though a followup CL will run two at once if the initial
connection attempt takes too long.
There was a fare bit pruned from a full CL, so some comments may
reference code not included in this initial CL.
- M net/BUILD.gn
- A net/socket/tcp_connect_job.cc
- A net/socket/tcp_connect_job.h
- A net/socket/tcp_connect_job_connector.cc
- A net/socket/tcp_connect_job_connector.h
- A net/socket/tcp_connect_job_unittest.cc
Code-Review: +1 by Kenichi Ishibashi
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |