Followups from #6561 Generalize socket-based event loop #7799

kpschoedel · 2021-06-21T21:35:09Z

Problem

Followups from #6561 Generalize socket-based event loop

Change overview

mDNS: Convert platform/Linux/MdnsImpl; remove Darwin stubs.
Change mCallbackData type to intptr_t (review feedback).
Comment for System::WakeEvent (this is minimal since it's
likely to change soon for issue Re-factor of how threading and event dispatch is handled in the SDK #7725).

Fixes #7758 Convert MDNS to WatchableSocket.

Testing

Manual sanity check of mDNS using chip-device-ctrl on Linux.
Otherwise, no change to functionality is intended.

src/platform/Linux/MdnsImpl.cpp

andy31415 · 2021-06-22T14:57:19Z

src/platform/Linux/MdnsImpl.cpp

@@ -145,19 +172,43 @@ AvahiWatch * Poller::WatchNew(int fd, AvahiWatchEvent event, AvahiWatchCallback
 {
    VerifyOrDie(callback != nullptr && fd >= 0);

-    mWatches.emplace_back(new AvahiWatch{ fd, event, 0, callback, context, this });
+    AvahiWatch * const watch = new AvahiWatch;


where is deletecalled for these objects?

Because of std::vector<std::unique_ptr<AvahiWatch>> mWatches;

auto watch = std::make_unique<AvahiWatch>() ?

andy31415 · 2021-06-22T14:59:13Z

src/system/WatchableSocketSelect.cpp

@@ -35,8 +35,8 @@
 namespace chip {
 namespace Mdns {
 // TODO(#5556): Convert MDNS to WatchableSocket.


Are these TODOs still relevant?

No, they're not; the remaining part is timer-related and pending #7725

mspang · 2021-06-22T19:28:44Z

src/platform/Linux/MdnsImpl.cpp

@@ -145,19 +172,43 @@ AvahiWatch * Poller::WatchNew(int fd, AvahiWatchEvent event, AvahiWatchCallback
 {
    VerifyOrDie(callback != nullptr && fd >= 0);

-    mWatches.emplace_back(new AvahiWatch{ fd, event, 0, callback, context, this });
+    AvahiWatch * const watch = new AvahiWatch;


auto watch = std::make_unique<AvahiWatch>() ?

mspang · 2021-06-22T19:29:41Z

src/inet/RawEndPoint.cpp

@@ -424,7 +424,7 @@ INET_ERROR RawEndPoint::Listen(IPEndPointBasis::OnMessageReceivedFunct onMessage

 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
    // Wait for ability to read on this endpoint.
-    mSocket.SetCallback(HandlePendingIO, this);
+    mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));


This requires a more casts and stronger casts, I wonder if it is worth it?

This was a request in the previous review, so you and Boris can fight it out :-)

It was a suggestion; Boris said he was OK either way. Above was just a question. Ultimately I think it is up to you.

Yeah, given what this diff looks like and that we have no non-pointer use cases for this closure, we should probably leave it as void* for now. If we ever end up with non-pointer use cases, we can think about how to be best address those then...

Added to my TODO list.

mspang · 2021-06-22T19:32:35Z

src/platform/Linux/MdnsImpl.h


 struct AvahiWatch
 {
+#if 1


Clean up dead else?

Followups from project-chip#6561 Generalize socket-based event loop #### Change overview * mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs. * Change `mCallbackData` type to `intptr_t` (review feedback). * Comment for `System::WakeEvent` (this is minimal since it's likely to change soon for issue project-chip#7725). Fixes project-chip#7758 _Convert MDNS to WatchableSocket._ #### Testing Manual sanity check of mDNS using chip-device-ctrl on Linux. Otherwise, no change to functionality is intended.

mspang · 2021-06-22T21:30:43Z

src/inet/RawEndPoint.cpp

@@ -424,7 +424,7 @@ INET_ERROR RawEndPoint::Listen(IPEndPointBasis::OnMessageReceivedFunct onMessage

 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
    // Wait for ability to read on this endpoint.
-    mSocket.SetCallback(HandlePendingIO, this);
+    mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));


It was a suggestion; Boris said he was OK either way. Above was just a question. Ultimately I think it is up to you.

mspang · 2021-06-22T21:43:58Z

src/platform/Linux/MdnsImpl.cpp

@@ -167,6 +208,7 @@ void Poller::WatchFree(AvahiWatch * watch)

 void Poller::WatchFree(AvahiWatch & watch)
 {
+    (void) watch.mSocket.ReleaseFD();


I'm a confused by this. WatchableSocket is normally expected to own the fd but not in the case of avahi?

If WatchableSocket is owning, why doesn't it close in its destructor?

If WatchableSocket is non-owning, why does it close the file descriptor in Close()?

If WatchableSocket non-owning but Close() is just a helper for release-and-close, should "release" call WatchableSocket::OnClose which does the actual detach-from-WatchableEventManager?

I'm a confused by this. WatchableSocket is normally expected to own the fd but not in the case of avahi?

Right. In the other cases (Inet and System::WakeEvent) the WatchableSocket effectively owns the fd, and replaces the fd from the point of view of the holder. In the Avahi case, Avahi itself owns the fd and lends it via AvahiPoll.watch_new and watch_free.

If WatchableSocket is owning, why doesn't it close in its destructor?

For the normal (Inet) case, it's contained inside an IPEndPointBasis which is not constructible or destructible. It's contained in order to avoid requiring separate allocation. It may be worth revisiting that if/when the System/Inet virtualization and pool allocation improvements happen.

If WatchableSocket is non-owning, why does it close the file descriptor in Close()?

Convenience for the common case.

If WatchableSocket non-owning but Close() is just a helper for release-and-close, should "release" call WatchableSocket::OnClose which does the actual detach-from-WatchableEventManager?

Yes, OnClose() should be renamed OnRelease().

I'm a confused by this. WatchableSocket is normally expected to own the fd but not in the case of avahi?

Right. In the other cases (Inet and System::WakeEvent) the WatchableSocket effectively owns the fd, and replaces the fd from the point of view of the holder. In the Avahi case, Avahi itself owns the fd and lends it via AvahiPoll.watch_new and watch_free.

Having a non-owning watcher separate from fd-owner makes the most sense to me and would fit well with avahi but understood there are other constraints here.

If WatchableSocket is owning, why doesn't it close in its destructor?

For the normal (Inet) case, it's contained inside an IPEndPointBasis which is not constructible or destructible. It's contained in order to avoid requiring separate allocation. It may be worth revisiting that if/when the System/Inet virtualization and pool allocation improvements happen.

Ahh, the pool should really run destructors on slots released back to it :(

If WatchableSocket is non-owning, why does it close the file descriptor in Close()?

Convenience for the common case.

If WatchableSocket non-owning but Close() is just a helper for release-and-close, should "release" call WatchableSocket::OnClose which does the actual detach-from-WatchableEventManager?

Yes, OnClose() should be renamed OnRelease().

Ah, I see that it does I was just confused by the name. Rename sounds good.

Having a non-owning watcher separate from fd-owner makes the most sense to me and would fit well with avahi but understood there are other constraints here.

I'll take a look at this when I do the remaining test and timer TODOs, which likely will be together with #7725. I don't know that it will make much difference; since we don't have weak fds, a watcher must be informed before a file is closed.

Yes, OnClose() should be renamed OnRelease().

Ah, I see that it does I was just confused by the name. Rename sounds good.

Ack, added to my TODO list.

bzbarsky-apple · 2021-06-23T02:02:36Z

src/inet/RawEndPoint.cpp

@@ -424,7 +424,7 @@ INET_ERROR RawEndPoint::Listen(IPEndPointBasis::OnMessageReceivedFunct onMessage

 #if CHIP_SYSTEM_CONFIG_USE_SOCKETS
    // Wait for ability to read on this endpoint.
-    mSocket.SetCallback(HandlePendingIO, this);
+    mSocket.SetCallback(HandlePendingIO, reinterpret_cast<intptr_t>(this));


Yeah, given what this diff looks like and that we have no non-pointer use cases for this closure, we should probably leave it as void* for now. If we ever end up with non-pointer use cases, we can think about how to be best address those then...

woody-apple · 2021-06-23T12:50:12Z

@saurabhst @msandstedt @Damian-Nordic @LuDuda ?

…roject-chip#7799) * #### Problem Followups from project-chip#6561 Generalize socket-based event loop #### Change overview * mDNS: Convert `platform/Linux/MdnsImpl`; remove Darwin stubs. * Change `mCallbackData` type to `intptr_t` (review feedback). * Comment for `System::WakeEvent` (this is minimal since it's likely to change soon for issue project-chip#7725). Fixes project-chip#7758 _Convert MDNS to WatchableSocket._ #### Testing Manual sanity check of mDNS using chip-device-ctrl on Linux. Otherwise, no change to functionality is intended. * review * review

boring-cyborg bot added darwin inet linux platform system labels Jun 21, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, saurabhst, turon, vivien-apple and woody-apple June 21, 2021 21:37

pullapprove bot added the review - pending label Jun 21, 2021

andy31415 added the Tested manually label Jun 22, 2021

andy31415 approved these changes Jun 22, 2021

View reviewed changes

andy31415 reviewed Jun 22, 2021

View reviewed changes

kpschoedel force-pushed the x5556-2 branch from 71beccb to d3fe536 Compare June 22, 2021 15:45

mspang reviewed Jun 22, 2021

View reviewed changes

kpschoedel added 3 commits June 22, 2021 15:52

review

0689964

review

3087153

kpschoedel force-pushed the x5556-2 branch from d3fe536 to 3087153 Compare June 22, 2021 20:01

mspang reviewed Jun 22, 2021

View reviewed changes

mspang approved these changes Jun 23, 2021

View reviewed changes

bzbarsky-apple approved these changes Jun 23, 2021

View reviewed changes

jmartinez-silabs approved these changes Jun 23, 2021

View reviewed changes

pullapprove bot removed the review - pending label Jun 23, 2021

pullapprove bot added the review - approved label Jun 23, 2021

andy31415 merged commit 9692e9a into project-chip:master Jun 23, 2021

kpschoedel deleted the x5556-2 branch July 13, 2021 15:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Followups from #6561 Generalize socket-based event loop #7799

Followups from #6561 Generalize socket-based event loop #7799

kpschoedel commented Jun 21, 2021 •

edited by andy31415

Loading

andy31415 Jun 22, 2021

kpschoedel Jun 22, 2021

mspang Jun 22, 2021

andy31415 Jun 22, 2021

kpschoedel Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

kpschoedel Jun 22, 2021

mspang Jun 22, 2021

bzbarsky-apple Jun 23, 2021

kpschoedel Jun 23, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

kpschoedel Jun 22, 2021

mspang Jun 23, 2021

kpschoedel Jun 23, 2021

bzbarsky-apple Jun 23, 2021

woody-apple commented Jun 23, 2021

Followups from #6561 Generalize socket-based event loop #7799

Followups from #6561 Generalize socket-based event loop #7799

Conversation

kpschoedel commented Jun 21, 2021 • edited by andy31415 Loading

Problem

Change overview

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woody-apple commented Jun 23, 2021

kpschoedel commented Jun 21, 2021 •

edited by andy31415

Loading