extract migration code [chromium/src : main]
Ryan Hamilton (Gerrit)
Ryan Hamilton added 11 comments![]( "Open in Gerrit")
A few first-pass comments. There is a LOT of new code here which I think we need to figure out how to test and how to break into somewhat smaller pieces.
class NET_EXPORT_PRIVATE QuicChromiumNetLog : public quic::QuicEventLog {I definitely don't think we should make a Chrome net log platform API. Instead, I think we should add various `OnFooHappened()` debug visitor methods which pass up the relevant information that Chrome-specific code can add to net-log.
const bool migrate_session_on_network_change_;It looks like a number of these fields are also in QuicNetworkChangeParameters. How 'bout we just store this struct in a member instead of duplicating?
class QUICHE_EXPORT QuicNetworkChangeObserver {As discussed yesterday this seems like a "MigrationManager" more than a "NetworkChangeObserver"
bool migrate_session_early;nit: brief comments for each of these. For example, what does "Early" mean here?
struct QuicNetworkChangeParameters {nit: If I understand this correctly, this is a collection of configuration variables for migration behavior. Is that right? If so maybe "QuicConnectionMigrationConfiguration" or some such.
// found in the LICENSE file.There is a LOT of new code here, and no unit tests. I think we should figure out how to write tests for this.
net_log_->AddEventWithInt64Params(Just reiterating my earlier point, instead of having a NetLog platform API for this, let's instead have a DebugVisitor (or something) for this class. Here we could call something like `debug_visitor_.OnNetworkConnected(QuicNetworkHandle network)`. Then the Chromium implementation can send this to the NetLog. This is how net/quic/quic_connection_logger.h works today.
if (!static_cast<net::QuicChromiumClientSession&>(session_.get())Since we're targeting this code to live in QUICHE, I don't think we can directly reference Chromium classes?
virtual void CreatePathValidationContext(Comment for this method. It sorta seems like it's a factory method but I don't think I understand what it's expected to do.
class QUICHE_EXPORT QuicPathContextManager {Class comment, please. I'm not clear how this is intended to be used.
Related details
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 1 comment![]( "Open in Gerrit")
if (!static_cast<net::QuicChromiumClientSession&>(session_.get())Since we're targeting this code to live in QUICHE, I don't think we can directly reference Chromium classes?
The plan is to add a new interface MigrateToNewPath() in QuicSession. Actually all the down casting to Chromium objects in this class means that the corresponding interfaces needs to be made into QUICHE interface.
Related details
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 8 comments![]( "Open in Gerrit")
class NET_EXPORT_PRIVATE QuicChromiumNetLog : public quic::QuicEventLog {I definitely don't think we should make a Chrome net log platform API. Instead, I think we should add various `OnFooHappened()` debug visitor methods which pass up the relevant information that Chrome-specific code can add to net-log.
Added DebugVisitor interface for this.
It looks like a number of these fields are also in QuicNetworkChangeParameters. How 'bout we just store this struct in a member instead of duplicating?
Done
As discussed yesterday this seems like a "MigrationManager" more than a "NetworkChangeObserver"
Renamed it to QuicConnectionMigrationManager.
nit: brief comments for each of these. For example, what does "Early" mean here?
Done
nit: If I understand this correctly, this is a collection of configuration variables for migration behavior. Is that right? If so maybe "QuicConnectionMigrationConfiguration" or some such.
Done
Just reiterating my earlier point, instead of having a NetLog platform API for this, let's instead have a DebugVisitor (or something) for this class. Here we could call something like `debug_visitor_.OnNetworkConnected(QuicNetworkHandle network)`. Then the Chromium implementation can send this to the NetLog. This is how net/quic/quic_connection_logger.h works today.
Done
Comment for this method. It sorta seems like it's a factory method but I don't think I understand what it's expected to do.
Done
Class comment, please. I'm not clear how this is intended to be used.
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Hamilton (Gerrit)
Ryan Hamilton added 21 comments![]( "Open in Gerrit")
A few more comments. I think we definitely need unit tests for the "quiche" classes.
shared_completion_callback = completion_callback.lock();Since completion_callback is a weak pointer, it's possible that this will return nullptr if the weak pointer is expired. This seems problematic.
std::shared_ptr<QuicPathContextManager::PathValidationContextCreationCallback>Do we need this to be a shared pointer or is there some way to work around that? I'm not sure what the motivation for reference counting is here.
}optional: consider moving all the simple getters down to the bottom of the list of methods
bool quic_connection_migration_successful() const {ditto
bool quic_connection_migration_attempted() const {nit: how 'bout just `migration_attempted` since the "quic connection" prefix is a bit redundant?
QuicNetworkHandle GetDefaultNetwork() const { return default_network_; }Since these are simple getters should they simply be `default_network()`?
quic::QuicSpdyClientSessionBase* session,For all of these pointers can we use absl Nullability annotations? go/totw/230
virtual ~QuicConnectionMigrationDebugVisitor() {}use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Please fix.
bool migrate_session_on_network_change;Would it make sense to set the default values for these members in the .h file?
QuicConnectionMigrationConfig::QuicConnectionMigrationConfig() = default;I think this leaves the POD members uninitialized. Is that expected?
const QuicConnectionMigrationConfig&) = default;Can we just do `= default` in the .h file for all 3 of these methods?
run_pending_callbacks_alarm_ =Can these be initialized in the initializer list instead of the constructor body?
run_pending_callbacks_alarm_->PermanentCancel();Is this necessary, or does the `QuicAlarm` destructor do the right thing?
// validation context, or with nullptr if this manager fail to create one.If `completion_callback` is only used to invoke the `OnPathValidationContextCreated()` method, can we pass in `absl::AnyInvokable`, or some such? (go/totw/191)
I'm also unclear why this needs to be a weak_ptr. I would think that could be handled, perhaps, in the subclass of `PathValidationContextCreationCallback`.
done_callback_;How is this member expected to be used?
QuicSession& session() { return session_.get(); }Why is this needed?
absl::string_view error);Please document these arguments. In particular, is `context` guaranteed to be non-null? Or will it be null if there is an error. Ditto about `error`. I wonder if there should be two methods, one for success which takes a non-null context, and one for error which takes an error message.
class PathValidationContextCreationCallback {If we need to keep this as a class, how 'bout naming it something like `Delegate`. Since it's nested class, it would be `QuicPathContextFactory::Delegate` for external uses.
In any case, please comment on this class. From reading the declaration here, I'm quite unclear how it is used. Specifically, this object is created with a `done_callback` but it's not obvious to me why that is needed/what it does.
class QUICHE_EXPORT QuicPathContextManager {If this is simply a class for creating objects, then "Factory" would be a good design pattern name. So perhaps `QuicPathContextFactory`?
inline constexpr QuicNetworkHandle kInvalidNetworkHandle{-1};Related details
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 7 comments![]( "Open in Gerrit")
shared_completion_callback = completion_callback.lock();Since completion_callback is a weak pointer, it's possible that this will return nullptr if the weak pointer is expired. This seems problematic.
This is called right after the caller created the callback object, so it's impossible that it becomes expired right away, thus the DCHECK above.
std::shared_ptr<QuicPathContextManager::PathValidationContextCreationCallback>Do we need this to be a shared pointer or is there some way to work around that? I'm not sure what the motivation for reference counting is here.
This is for the weak_ptr of completion_callback passed into QuicPathContextManager, given that the path context creation can be async.
const QuicConnectionMigrationConfig&) = default;Can we just do `= default` in the .h file for all 3 of these methods?
I ran into compilation errors if inlining the definition of these constructors and destructor, which looks like Chromium specific. I can try to inline them once moving these classes into QUICHE.
run_pending_callbacks_alarm_->PermanentCancel();Is this necessary, or does the `QuicAlarm` destructor do the right thing?
~QuicAlarm() doesn't cancel itself given cancellation implementation is in subclasses. But it does increment a code counter if itself is not cancelled. So it's better we explicit cancel all the alarms like what QuicConnection does.
// validation context, or with nullptr if this manager fail to create one.If `completion_callback` is only used to invoke the `OnPathValidationContextCreated()` method, can we pass in `absl::AnyInvokable`, or some such? (go/totw/191)
I'm also unclear why this needs to be a weak_ptr. I would think that could be handled, perhaps, in the subclass of `PathValidationContextCreationCallback`.
We can use absl::AnyInvokable to replace the std::function done_callback_ inside this class, but the reason why we wrap the actual callback inside PathValidationContextCreationCallback and pass in a weak_ptr of it here is because this call can be async, and its caller which owns the completion_callback may gets destructed before CreatePathValidationContext() is called. And PathValidationContextCreationCallback can also retain a handle to the session object.
done_callback_;How is this member expected to be used?
This is called after a path context object is created to pass it back to QuicConnectionMigrationManager. This might be a function to kick off path validation or immediate migration depending on where it is called.
QuicSession& session() { return session_.get(); }Why is this needed?
QuicChromiumPathContextManager::CreatePathValidationContextWithSocket() needs to access the chromium session object for net log and to create chromium packet reader. Wrapping it into PathValidationContextCreationCallback is just a convenient way of passing the session object around given its life time is already managed.
Related details
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 14 comments![]( "Open in Gerrit")
optional: consider moving all the simple getters down to the bottom of the list of methods
Done
bool quic_connection_migration_successful() const {Dan Zhangditto
Done
nit: how 'bout just `migration_attempted` since the "quic connection" prefix is a bit redundant?
Done
QuicNetworkHandle GetDefaultNetwork() const { return default_network_; }Dan ZhangSince these are simple getters should they simply be `default_network()`?
Done
For all of these pointers can we use absl Nullability annotations? go/totw/230
Done
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Please fix.
Done
Would it make sense to set the default values for these members in the .h file?
Done
QuicConnectionMigrationConfig::QuicConnectionMigrationConfig() = default;Dan ZhangI think this leaves the POD members uninitialized. Is that expected?
I initialized them with some sort of defaults now.
run_pending_callbacks_alarm_ =Dan ZhangCan these be initialized in the initializer list instead of the constructor body?
Done
Dan ZhangWhy is this needed?
QuicChromiumPathContextManager::CreatePathValidationContextWithSocket() needs to access the chromium session object for net log and to create chromium packet reader. Wrapping it into PathValidationContextCreationCallback is just a convenient way of passing the session object around given its life time is already managed.
Not needed any more after this class being made per session
Please document these arguments. In particular, is `context` guaranteed to be non-null? Or will it be null if there is an error. Ditto about `error`. I wonder if there should be two methods, one for success which takes a non-null context, and one for error which takes an error message.
Done
class PathValidationContextCreationCallback {If we need to keep this as a class, how 'bout naming it something like `Delegate`. Since it's nested class, it would be `QuicPathContextFactory::Delegate` for external uses.
In any case, please comment on this class. From reading the declaration here, I'm quite unclear how it is used. Specifically, this object is created with a `done_callback` but it's not obvious to me why that is needed/what it does.
Done
If this is simply a class for creating objects, then "Factory" would be a good design pattern name. So perhaps `QuicPathContextFactory`?
Done
inline constexpr QuicNetworkHandle kInvalidNetworkHandle{-1};nit: ` = -1;` to be simpler.
Done
Related details
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 3 comments![]( "Open in Gerrit")
Dan ZhangA few more comments. I think we definitely need unit tests for the "quiche" classes.
I added some unit tests for migration manager, will continue working on more test cases.
virtual ~QuicConnectionMigrationDebugVisitor() {}Dan Zhanguse '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: running clang-tidy on this file failed; this diagnostic might be incorrect as a result.)
(Lint observed on `android-clang-tidy-rel` and `linux-clang-tidy-rel`)
Please fix.
Done
QuicSession& session() { return session_.get(); }Dan ZhangWhy is this needed?
Dan ZhangQuicChromiumPathContextManager::CreatePathValidationContextWithSocket() needs to access the chromium session object for net log and to create chromium packet reader. Wrapping it into PathValidationContextCreationCallback is just a convenient way of passing the session object around given its life time is already managed.
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 1 comment![]( "Open in Gerrit")
Related details
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ryan Hamilton (Gerrit)
Ryan Hamilton added 22 comments![]( "Open in Gerrit")
Ok, I think we're getting there! The tests, in particular, look promising.
QuicConnectionMigrationManager* migration_manager() {Since this is always non-null can this return a non-const ref?
virtual bool PrepareForMigratingToPath(nit: "ForMigrating" => "ForMigration" to be consistent with "OnMigrationToPathDone".
// network.Since this method potentially changes the network, but `QuicPathValidationContext` does not include the network handle, this method should probably take a `QuicClientPathValidationContext`.
// Called when probing the server's preferred address from a differnt port"differnt" is a possible misspelling of "different".
Please fix.
// Called when NetworkChangeNotifier notifies observers of a newlynit: I think `NetworkChangeNotifier` is Chrome-specific so we probably want to rephrase this (and other) comment to say something like:
Called when a new network is connected by the operating system...
(or some such)
// migration.Is this specifically for probe-based migration? If so, let's mention that.
// |migration_manager_| should out live |this|.Would be good to mention this in the constructor comment.
QuicConnectionMigrationConfig::~QuicConnectionMigrationConfig() = default;Maybe we can move this to a stand-alone .cc file to make this one smaller. (or just put these in the .h file)
auto* context = static_cast<QuicClientPathValidationContext*>(I'm probably missing something, but what ensure that the connection's context is a `QuicClientPathValidationContext`?
QuicSpdyClientSessionWithMigration::QuicSpdyClientSessionWithMigration(Maybe we can move this to a stand-alone .cc file to make this one smaller.
static_cast<QuicClientPathValidationContext&>(*path_context)I think that perhaps this method should take a `QuicClientPathValidationContext` instead of a `QuicPathValidationContext` so that this cast is not needed.
// migrated session back to the default network. Migration singals delivered"singals" is a possible misspelling of "signals".
Please fix.
// in the following order (alternate network is always availabe):"availabe" is a possible misspelling of "available".
Please fix.
virtual void ForceWriteBlockage(bool write_blocked) = 0;nit: "Blockage" => "Blocked", I think.
class QUICHE_EXPORT QuicForceBlockablePacketWriter : public QuicPacketWriter {nit: Brief class comment, please.
// |completion_callback|.OnPathValidationContextCreated() can be called eitherShould this method take a `completion_callback`, or should the comment be modified to say reference `result_delegate` instead?
// session_ should outlive the callback.nit: What is `session_`?
absl::string_view error);Instead of having CreationResultDelegate be a concrete class which takes a Callback as an argument, what about having this class be an interface with pure virtual `OnCreation()` methods. Then the caller can create a subclass of this interface with whatever logic they need. That might eliminate a bit of redundant layering.
Alternatively, we could remove this class completely and just pass the callback into `CreatePathValidationContext()` directly (though I think the delegate route is probably a bit cleaner).
// in which case |error| will be populated with details.Instead of a single method with two modes and two parameters, it might make sense to have `OnCreationSucceeded(context)` and `OnCreationFailed(error)`.
virtual bool ShouldConnectionOwnWriter() const = 0;What are the semantics of this method, and out of curiosity why is it needed?
class QUICHE_EXPORT QuicClientPathValidationContextnit: Class comment, please. In particular the only new field in this class is the network handle, but the name is `Client`, so it would be good to explain why the network handle is client-specific.
Also, I wonder if it might make sense to name this `QuicMigrationContext` and instead of extending `QuicPathValidationContext` make it have a member of that type, since it seems a bit like composition is a better fit here than inheritance.
Related details
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 24 comments![]( "Open in Gerrit")
QuicConnectionMigrationManager* migration_manager() {Since this is always non-null can this return a non-const ref?
Done
virtual bool PrepareForMigratingToPath(nit: "ForMigrating" => "ForMigration" to be consistent with "OnMigrationToPathDone".
Done
// network.Since this method potentially changes the network, but `QuicPathValidationContext` does not include the network handle, this method should probably take a `QuicClientPathValidationContext`.
Done
// Called when probing the server's preferred address from a differnt port"differnt" is a possible misspelling of "different".
Please fix.
Done
// Called when NetworkChangeNotifier notifies observers of a newlynit: I think `NetworkChangeNotifier` is Chrome-specific so we probably want to rephrase this (and other) comment to say something like:
Called when a new network is connected by the operating system...
(or some such)
Done
// migration.Is this specifically for probe-based migration? If so, let's mention that.
Done
// |migration_manager_| should out live |this|.Would be good to mention this in the constructor comment.
Done
QuicConnectionMigrationConfig::~QuicConnectionMigrationConfig() = default;Maybe we can move this to a stand-alone .cc file to make this one smaller. (or just put these in the .h file)
Chromium doesn't allow inlined member function definition for complex classes, so I have to move even the default constructor and destructor into .cc to avoid chromium specific build error. But once the code gets moved into QUICHE, we can inline them.
auto* context = static_cast<QuicClientPathValidationContext*>(I'm probably missing something, but what ensure that the connection's context is a `QuicClientPathValidationContext`?
This is in OnNetworkDisconnected(), so it's a client connection. Any on-going path validation's context is provided by the context factory and thus guaranteed to be a QuicClientPathValidationContext.
QuicSpdyClientSessionWithMigration::QuicSpdyClientSessionWithMigration(Maybe we can move this to a stand-alone .cc file to make this one smaller.
Done
static_cast<QuicClientPathValidationContext&>(*path_context)I think that perhaps this method should take a `QuicClientPathValidationContext` instead of a `QuicPathValidationContext` so that this cast is not needed.
Done
// migrated session back to the default network. Migration singals delivered"singals" is a possible misspelling of "signals".
Please fix.
Done
// in the following order (alternate network is always availabe):"availabe" is a possible misspelling of "available".
Please fix.
Done
virtual void ForceWriteBlockage(bool write_blocked) = 0;nit: "Blockage" => "Blocked", I think.
The bool argument, when false, will undo any previous force blocking. Naming it ForceWriteBlocked() is misleading. Admittedly the current name sounds weird too...
class QUICHE_EXPORT QuicForceBlockablePacketWriter : public QuicPacketWriter {nit: Brief class comment, please.
Done
There is a LOT of new code here, and no unit tests. I think we should figure out how to write tests for this.
Done
if (!static_cast<net::QuicChromiumClientSession&>(session_.get())Since we're targeting this code to live in QUICHE, I don't think we can directly reference Chromium classes?
The plan is to add a new interface MigrateToNewPath() in QuicSession. Actually all the down casting to Chromium objects in this class means that the corresponding interfaces needs to be made into QUICHE interface.
I believe the extracted code no longer references chromium classes any more.
// |completion_callback|.OnPathValidationContextCreated() can be called eitherShould this method take a `completion_callback`, or should the comment be modified to say reference `result_delegate` instead?
Updated the comment
// session_ should outlive the callback.nit: What is `session_`?
Outdated and removed.
absl::string_view error);Instead of having CreationResultDelegate be a concrete class which takes a Callback as an argument, what about having this class be an interface with pure virtual `OnCreation()` methods. Then the caller can create a subclass of this interface with whatever logic they need. That might eliminate a bit of redundant layering.
Alternatively, we could remove this class completely and just pass the callback into `CreatePathValidationContext()` directly (though I think the delegate route is probably a bit cleaner).
Changed it into an abstract interface which is cleaner to me.
// in which case |error| will be populated with details.Instead of a single method with two modes and two parameters, it might make sense to have `OnCreationSucceeded(context)` and `OnCreationFailed(error)`.
Done
virtual bool ShouldConnectionOwnWriter() const = 0;What are the semantics of this method, and out of curiosity why is it needed?
QuicConnection might not necessarily owns the writer. It's up to the platform specific code to call into its constructor. So when we call MigrateToPath() on it during migration, we also need to be consistent about the ownership of the new writer. Currently this method takes a bool to indicate whether connection should take the ownership. Since the path context also has its platform-specific implementation, I think it's a good place to retain the writer ownership preference here. In Chromium, QuicConnection will take the ownership of the initial writer and all the newer writers after migration.
class QUICHE_EXPORT QuicClientPathValidationContextnit: Class comment, please. In particular the only new field in this class is the network handle, but the name is `Client`, so it would be good to explain why the network handle is client-specific.
Also, I wonder if it might make sense to name this `QuicMigrationContext` and instead of extending `QuicPathValidationContext` make it have a member of that type, since it seems a bit like composition is a better fit here than inheritance.
The down side of composition is that network handle won't be passed along during async path validation as the current interface takes a QuicPathValidationContext object.
// validation context, or with nullptr if this manager fail to create one.If `completion_callback` is only used to invoke the `OnPathValidationContextCreated()` method, can we pass in `absl::AnyInvokable`, or some such? (go/totw/191)
I'm also unclear why this needs to be a weak_ptr. I would think that could be handled, perhaps, in the subclass of `PathValidationContextCreationCallback`.
We can use absl::AnyInvokable to replace the std::function done_callback_ inside this class, but the reason why we wrap the actual callback inside PathValidationContextCreationCallback and pass in a weak_ptr of it here is because this call can be async, and its caller which owns the completion_callback may gets destructed before CreatePathValidationContext() is called. And PathValidationContextCreationCallback can also retain a handle to the session object.
Removed the usage of weak_ptr in favor of making the session own the context factory.
Related details
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ali Beyad (Gerrit)
Ali Beyad added 6 comments![]( "Open in Gerrit")
const DatagramClientSocket* GetDefaultSocket() const;same here, this just seems moved around, if we can put it back in the original order so it doesn't show up in the diff?
quic::QuicTimeDelta TimeSinceLastStreamClose() override;these new functions could use comments
void WriteToNewSocket();this seems unchanged, just moved around, can we put it back in the same order so it doesn't show up in the diff?
bool migrate_sessions_on_network_change_v2,nit: this CL already has a lot going on, so we should probably save other improvements like this one (thanks for spotting and fixing this btw!) into a separate CL.
class QUIC_EXPORT QuicConnectionMigrationDebugVisitor {is
void QuicSpdyClientSessionWithMigration::SetMigrationDebugVisitor(what is the purpose of the QuicConnectionMigrationDebugVisitor? I don't see it used anywhere.
Related details
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 7 comments![]( "Open in Gerrit")
const DatagramClientSocket* GetDefaultSocket() const;same here, this just seems moved around, if we can put it back in the original order so it doesn't show up in the diff?
Done
quic::QuicTimeDelta TimeSinceLastStreamClose() override;these new functions could use comments
The comment is added to the virtual interface. Here are just the implementations.
class QUIC_EXPORT QuicConnectionMigrationDebugVisitor {is
typo?
void QuicSpdyClientSessionWithMigration::SetMigrationDebugVisitor(what is the purpose of the QuicConnectionMigrationDebugVisitor? I don't see it used anywhere.
It is the chromium net log. See quic_connection_logger.h
virtual void ForceWriteBlockage(bool write_blocked) = 0;Dan Zhangnit: "Blockage" => "Blocked", I think.
The bool argument, when false, will undo any previous force blocking. Naming it ForceWriteBlocked() is misleading. Admittedly the current name sounds weird too...
Any thoughts on this?
Dan ZhangWhat are the semantics of this method, and out of curiosity why is it needed?
QuicConnection might not necessarily owns the writer. It's up to the platform specific code to call into its constructor. So when we call MigrateToPath() on it during migration, we also need to be consistent about the ownership of the new writer. Currently this method takes a bool to indicate whether connection should take the ownership. Since the path context also has its platform-specific implementation, I think it's a good place to retain the writer ownership preference here. In Chromium, QuicConnection will take the ownership of the initial writer and all the newer writers after migration.
Same as network() discussed above.
Dan Zhangnit: Class comment, please. In particular the only new field in this class is the network handle, but the name is `Client`, so it would be good to explain why the network handle is client-specific.
Also, I wonder if it might make sense to name this `QuicMigrationContext` and instead of extending `QuicPathValidationContext` make it have a member of that type, since it seems a bit like composition is a better fit here than inheritance.
The down side of composition is that network handle won't be passed along during async path validation as the current interface takes a QuicPathValidationContext object.
Discussed offline, we will move these 2 new interfaces into the base class QuicPathValidationContext when the code get moved into QUICHE. I added TODO for this.
Related details
- Ali Beyad
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 2 comments![]( "Open in Gerrit")
void WriteToNewSocket();Dan Zhangthis seems unchanged, just moved around, can we put it back in the same order so it doesn't show up in the diff?
Done
Dan ZhangHow is this member expected to be used?
This is called after a path context object is created to pass it back to QuicConnectionMigrationManager. This might be a function to kick off path validation or immediate migration depending on where it is called.
Changed this to me virtual interface
Related details
- Ali Beyad
- Dan Zhang
- Ryan Hamilton
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 1 comment![]( "Open in Gerrit")
bool migrate_sessions_on_network_change_v2,nit: this CL already has a lot going on, so we should probably save other improvements like this one (thanks for spotting and fixing this btw!) into a separate CL.
I got complain about existing typo otherwise, maybe because I touched the related code.
Ryan Hamilton (Gerrit)
Ryan Hamilton added 1 comment![]( "Open in Gerrit")
virtual void ForceWriteBlockage(bool write_blocked) = 0;Dan Zhangnit: "Blockage" => "Blocked", I think.
Dan ZhangThe bool argument, when false, will undo any previous force blocking. Naming it ForceWriteBlocked() is misleading. Admittedly the current name sounds weird too...
Any thoughts on this?
Hm. I'm not sure that `ForceWriteBlocked(false)` sounds any more or less misleading than `ForWriteBlockage(false)`. To my ear they're pretty similar, the only difference being that "Blocked" is parallel to "IsWriteBocked" whereas "Blockage" is similar but different.
Related details
- Ali Beyad
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dan Zhang (Gerrit)
Dan Zhang added 1 comment![]( "Open in Gerrit")
virtual void ForceWriteBlockage(bool write_blocked) = 0;Dan Zhangnit: "Blockage" => "Blocked", I think.
Dan ZhangThe bool argument, when false, will undo any previous force blocking. Naming it ForceWriteBlocked() is misleading. Admittedly the current name sounds weird too...
Ryan HamiltonAny thoughts on this?
Hm. I'm not sure that `ForceWriteBlocked(false)` sounds any more or less misleading than `ForWriteBlockage(false)`. To my ear they're pretty similar, the only difference being that "Blocked" is parallel to "IsWriteBocked" whereas "Blockage" is similar but different.
Okay, renamed the method and its argument to be less ambigious.
Related details
- Ali Beyad
- Dan Zhang
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |