network: delegate file event creation to io handle #12720
Conversation
florincoras
requested review from
alyssawilk,
htuch,
lizan,
mattklein123 and
PiotrSikora
as code owners
Avoid exposing the fd and allow the io handle to build custom file events. Signed-off-by: Florin Coras <fcoras@cisco.com>
|
cc @antoniovicente would be good to get your review on this! |
|
/assign @antoniovicente |
Signed-off-by: Florin Coras <fcoras@cisco.com>
| * should initially listen on. | ||
| * @return @ref Event::FileEventPtr | ||
| */ | ||
| virtual Event::FileEventPtr createFileEvent(Event::Dispatcher& dispatcher, Event::FileReadyCb cb, |
There was a problem hiding this comment.
The long term plan continues to be to completely remove "fd" from all top level dispatcher interfaces, right? My main concern is there is nothing right now that prevents not calling the original FileEvent factory function with the IoHandle fd, right? Can we add relevant comments around usage, TODOs for eventual removal, warnings, etc.? My quick take would be that if we have a similar function on the filesystem interface we might be able to completely remove the public interface that allows file event creation from fd sooner rather than later (I'm not sure what else actually uses that factory currently)? wdyt?
There was a problem hiding this comment.
That's exactly right. The goal is to use IoHandles instead of fds whenever possible.
Apart from the sockets, others that use the dispatcher's original FileEvent factory function are:
WatcherImpl, which creates inotify events using raw fdsDnsImpl, which relies on raw fds from AresHotRestartingParent, which also uses raw fds
In theory, we can change all of these, including IoSocketHandleImpl to allocate FileEventPtrs directly and then completely remove the dispatcher's public interface. The only thing we'll miss out on is the isThreadSafe() check, but maybe we can make the function public? I can take care of this but should we do it as part of this PR or a separate one?
Finally, out of those 3, maybe we should update at least DnsImpl to use IoHandles (not sure if that's easy).
There was a problem hiding this comment.
In theory, we can change all of these, including IoSocketHandleImpl to allocate FileEventPtrs directly and then completely remove the dispatcher's public interface. The only thing we'll miss out on is the isThreadSafe() check, but maybe we can make the function public? I can take care of this but should we do it as part of this PR or a separate one?
I would prefer we just do it now if you are up for it, but if you feel strongly we can do it as a fast follow up?
Finally, out of those 3, maybe we should update at least DnsImpl to use IoHandles (not sure if that's easy).
Yeah that would be nice. Maybe add a TODO there for follow up later?
There was a problem hiding this comment.
Was about to start work on it but it turns out FileEventImpl depends on DispatcherImpl and all is bundled in //source/common/event:dispatcher_lib. Trying to add the latter as dependency to the default_socket_interface_lib quickly runs into circular dependencies. I guess this is because we don't want to expose the scheduler's implementation, i.e., libevent's event_base, in the dispatcher's interface.
Unless there's a smarter way, we'll have have to separate FileEventImpl from DispatcherImpl and that might not be trivial (they'd have to exchange libevent specific objects over generic apis). The alternative would be to still use the dispatcher as a FileEvent factory and work towards removing the fd from IoHandle (more of that here and #12736). But that exercise is also not trivial because "higher level transports" like tls and quic do want to see the raw fds. So it might be some time until we cleanup everything.
What option would you prefer?
There was a problem hiding this comment.
Eliminating use of the IoHandle::fd() method seems like progress.
Direct creation of FileEvents instead of going through the dispatcher would reduce out ability to mock out FileEvent operations or inject test versions that can easily be marked read/write blocked, unless we add some factory interface similar to the one provided by the dispatchers today.
There was a problem hiding this comment.
Yes potentially we can just have a format/lint checker while we do further cleanups.
There was a problem hiding this comment.
and yes maybe also rename fdDoNotUse() or something until we fix it all.
There was a problem hiding this comment.
Works for me! Can we do it after all the PRs are merged to limit impact, i.e., change name and then delete them?
There was a problem hiding this comment.
Thanks! Added to my todo list :-)
| file_event_ = dispatcher_.createFileEvent( | ||
| ConnectionImpl::ioHandle().fd(), [this](uint32_t events) -> void { onFileEvent(events); }, | ||
| trigger, Event::FileReadyType::Read | Event::FileReadyType::Write); | ||
| file_event_ = socket_->ioHandle().createFileEvent( |
There was a problem hiding this comment.
nit: The RELEASE_ASSERT in line 62 above may make more sense in IoSocketHandleImpl::createFileEvent, DispatcherImpl::createFileEvent or FileEventImpl so we can remove the call to the ioHandle fd method.
There was a problem hiding this comment.
I was actually planning a separate PR that removes all of those asserts! Should I do this one in particular here or is it fine if we postpone it to the next PR?
Avoid exposing the fd and allow the io handle to build custom file events.
Signed-off-by: Florin Coras fcoras@cisco.com
Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a