Add utils to create and prepare socket for tcp client #31009
Conversation
Vignesh2208
added
lang/core
release notes: no
grpc-checks
bot
added
bloat/none
per-call-memory/neutral
per-channel-memory/neutral
labels
…f4b03 Automated fix for refs/heads/ee-tcp-posix-fork
| return status; | ||
| } | ||
|
|
||
| auto is_unix_socket = [](const EventEngine::ResolvedAddress& addr) -> bool { |
There was a problem hiding this comment.
I don't see why this needs to be a lambda or function, the logic can be inlined here.
| close(sock.Fd()); | ||
| } | ||
| }); | ||
| status = sock.SetSocketNonBlocking(1); |
There was a problem hiding this comment.
I'd recommend generalizing the RETURN_IF_ERROR macro in src/core/ext/transport/binder/wire_format/wire_reader_impl.cc, it will make this much more readable.
There was a problem hiding this comment.
No, this just made another copy of it. My recommendation is to put it in some central location (so it exists exactly once somewhere) and reuse it across gRPC.
There was a problem hiding this comment.
Moved it to src/core/lib/gprpp/status_helper.h and modified all use cases.
| PosixTcpOptions& options, const EventEngine::ResolvedAddress& target_addr, | ||
| EventEngine::ResolvedAddress& output_mapped_target_addr) { | ||
| PosixSocketWrapper::DSMode dsmode; | ||
| absl::StatusOr<PosixSocketWrapper> status; |
There was a problem hiding this comment.
Suggest renaming this variable to posix_socket_wrapper. Best practice is to name StatusOr variables as the variable type, not the status. See http://go/totw/labs/statusor-patterns-and-antipatterns
| // unconnected TCP client fd. | ||
| // options: may contain custom tcp settings for the fd. | ||
| // target_addr: the destination address. | ||
| // output_mapped_target_addr: A out parameter. It is target_addr mapped to an |
There was a problem hiding this comment.
Can output_mapped_target_addr be replaced by a member of the PosixSocketWrapper? Should it always map to PeerAddress? It would be a cleaner API to have either a return value or output parameters, not both.
There was a problem hiding this comment.
I would like to keep the memory footprint of PosixSocketWrapper very small - currently it has only one member variable i.e the socket descriptor and all the operations it supports are stateless. This allows creating socket wrappers at will and using them to manipulate socket flags. The peer address is requested only once and I dont think we should be storing it in the socket wrapper. We can store it in the endpoint code that requests for it.
Shall I modify this to return an absl::StatusOr<std::tuple<PosixSocketWrapper, ResolvedAddress>> instead ?
There was a problem hiding this comment.
I see. In that case, I'd suggest a return struct, per https://abseil.io/tips/176
| #include "src/core/lib/gprpp/host_port.h" | ||
|
|
||
| #ifdef GRPC_HAVE_UNIX_SOCKET | ||
| #include <sys/un.h> | ||
| #endif | ||
|
|
||
| // If expr evaluates to absl Not-OK status, then execute expr2 and return the | ||
| // evaluated status. | ||
| #define RETURN_IF_ERROR(expr, expr2) \ |
There was a problem hiding this comment.
The single-argument version is more general. I'd recommend keeping the absl::cleanup as you had it before, and standardizing on a general solution that works across gRPC.
There was a problem hiding this comment.
Moved it to src/core/lib/gprpp/status_helper.h file now.
| sock.TrySetSocketTcpUserTimeout(options, true); | ||
| } | ||
| RETURN_IF_ERROR(sock.SetSocketNoSigpipeIfPossible()); | ||
| auto status = sock.ApplySocketMutatorInOptions( |
There was a problem hiding this comment.
nit: can't the end of this function be simplified to:
RETURN_IF_ERROR(sock.ApplySocketMutatorInOptions(...));
close_fd = false;
return absl::OkStatus();
| @@ -301,10 +303,33 @@ class PosixSocketWrapper { | |||
| const experimental::EventEngine::ResolvedAddress& addr, int type, | |||
| int protocol, DSMode& dsmode); | |||
|
|
|||
| struct PosixSocketCreateResult; | |||
There was a problem hiding this comment.
This does not need to be forward-declared. See the style guide advice here: https://google.github.io/styleguide/cppguide.html#Forward_Declarations
There was a problem hiding this comment.
It wasn't letting me compile otherwise because the PosixSocketWrapper definition is not complete at that point. So I moved the CreateAndPrepareTcpClientSocket function outside the class definition and made it non static.
There was a problem hiding this comment.
Re-introduced it again after our discussion.
| const PosixTcpOptions& options, | ||
| const EventEngine::ResolvedAddress& target_addr) { | ||
| PosixSocketWrapper::DSMode dsmode; | ||
| EventEngine::ResolvedAddress output_mapped_target_addr; |
There was a problem hiding this comment.
This argument is no longer output-mapped. Please change the name.
There was a problem hiding this comment.
Changed to mapped_target_addr
copybara-service
bot
added
the
imported
Forks the grpc_tcp_client_prepare_fd function defined in src/core/lib/iomgr/tcp_client_posix.h