Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an unused implementation NoOpTransportSocketCallbacks #4172

Closed
wants to merge 9 commits into from

Conversation

danzh2010
Copy link
Contributor

Signed-off-by: Dan Zhang danzh@chromium.com

Description:
Add an implementation of TransportSocketCallbacks which does nothing when setReadBufferReady() or raiseEvent() is call. This implementation is supposed to be used in TransportSocket implementation which wraps another socket object. In this case, the underlying socket's callbacks should be suppressed.

Risk Level: Low (not enabled in main)
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A

from underlying socket in a TransportSocket implementation which wraps another socket.
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
Signed-off-by: Dan Zhang <danzh@google.com>
@danzh2010 danzh2010 requested a review from lizan as a code owner August 15, 2018 23:21
@lizan
Copy link
Member

lizan commented Aug 15, 2018

This implementation is supposed to be used in TransportSocket implementation which wraps another socket object. In this case, the underlying socket's callbacks should be suppressed.

I'm not sure how common this is. In capture TransportSocket those underlying socket's callbacks are NOT suppressed.

Seems the purpose of this is extracting the callbacks in #4153, am I missed anything?

/**
* @return the TransportSocketCallbacks object passed in through setTransportSocketCallbacks().
*/
virtual TransportSocketCallbacks* callbacks() PURE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need this? (see comments on NoOpTransportSocketCallbacks too)

*/
class NoOpTransportSocketCallbacks : public Network::TransportSocketCallbacks {
public:
explicit NoOpTransportSocketCallbacks(Network::TransportSocket* parent) : parent_(parent) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All you need is parent_->callbacks(), so why just not taking a Network::TransportSocketCallbacks& as parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on the timing when NoOpTransportSocketCallbacks is initialized and parent socket's setTransportSocketCallbacks() is called. In #4153 it is initialized before parent knows its callbacks. It can take a TransportSocketCallbacks** too, but I feel not much more simpler.

@danzh2010
Copy link
Contributor Author

This class is for #4153 as well as some internal usage where we need to wrap a transport socket.
BTW. I messed up the DCO sign-off for this PR. I will send out another one for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants