sock: initial definitions for asynchronous event handling by miri64 · Pull Request #11723 · RIOT-OS/RIOT

miri64 · 2019-06-19T13:03:38Z

Contribution description

This represents another approach to asynchronous sock as outlined in #8149 (comment). Apart from the necessary definitions for all basic sock types, this PR also contains example implementations for event.h and msg.h which can also be separated out into another PR.

Testing procedure

No implementation in this PR, just API (and a implementations using this unimplemented API ;-)).

Issues/PRs references

Alternative to #8149.

kaspar030 · 2019-06-19T13:21:20Z

Nice move forward!

Some thoughts (only brief look):

if we introduce a generic sock_t first, that all other sock types (udp, tcp, ip, ...) inherit from, the API gets a lot smaller, and the async handling part can be written once, for all of them. And IMO, as now we're starting to get shared funtionality, this would actually make sense to introduce.
I'm envisioning only a couple of ops, like read(), write(), close(), status(), setup_async_event() .
the event type needs to be OR'ed.

If I see correctly, there's one "ctx" per sock, and thus one event object. If now there are multiple events (CAN_READ | CLOSED), the second that arrives overwrites the first. The event should be used as bitfield, and it should be set in a way that even if the event is already queued, a subsequent bit that gets set can still be read.

Do you think we need to go all generic with the callback method?

I think we might get away with just ifdef'ing support for the different event methods, without resorting to custom types.
Like, only #ifdef MODULE_SOCK_ASYNC_EVENT, the necessary fields are added to "sock_x_t", and the helper functions are included.

miri64 · 2019-06-19T14:08:24Z

if we introduce a generic sock_t first, that all other sock types (udp, tcp, ip, ...) inherit from, the API gets a lot smaller, and the async handling part can be written once, for all of them. And IMO, as now we're starting to get shared funtionality, this would actually make sense to introduce.
I'm envisioning only a couple of ops, like read(), write(), close(), status(), setup_async_event() .

I was thinking about this, actually even had some header definitions for that however those were transparent to the lower levels of the implementation and thus did not really simplify things. Everything beyond that would rather be a completely new API, which would not sock and thus has nothing to do with my proposal.

the event type needs to be OR'ed.

They will be. By the lower level implementation. As you can see for sock_event_type_t, the values are OR'able ;-).

If I see correctly, there's one "ctx" per sock, and thus one event object.

ctx is very implementation specific, as you may have noticed, so it could also be a queue of objects.

If now there are multiple events (CAN_READ | CLOSED), the second that arrives overwrites the first. The event should be used as bitfield, and it should be set in a way that even if the event is already queued, a subsequent bit that gets set can still be read.

There are ways to prevent that (e.g. making sure the callback is only once and then again after an event was handled)

Do you think we need to go all generic with the callback method?

As pointed out above: I'm against changing the current API (apart from amendments and minor changes) at this point.

I think we might get away with just ifdef'ing support for the different event methods, without resorting to custom types.
Like, only #ifdef MODULE_SOCK_ASYNC_EVENT, the necessary fields are added to "sock_x_t", and the helper functions are included.

That is the idea of SOCK_HAS_ASYNC

miri64 · 2019-06-19T18:39:13Z

Another reason against using a "generic sock" here would be that it then wouldn't be applicable for TCP listener queues.

miri64 · 2019-06-19T19:49:04Z

I added another example implementation using msg_t which could come in handy at some point for fixing #8130

haukepetersen · 2019-10-08T07:01:44Z

Very nice, can't wait to have axync sock!

I am not 100% sure if I understood the code correctly, but I don't like the direct mapping to event and msg. I would think that it is more flexible and future proof, if we would introduce a classical callback interface without any dependency to any IPC implemenation as base.

We can then use this 'lower-level' to implement the usual convenience wrappers for concrete IPC implementations (msg, event, thread_flags) on top of that, pretty much like the xtimer interface. This way we can easily map any IPC that might pop up in the future, but also users are able to decide on their own way to handle sock events... Also it makes it easy to select the needed functionality, as in 'USEMODULE sock_async_thread_flags` etc...

If this is already the case and I just did not read the code of this PR correctly then never mind :-)

miri64 · 2019-10-08T07:05:33Z

I am not 100% sure if I understood the code correctly, but I don't like the direct mapping to event and msg. I would think that it is more flexible and future proof, if we would introduce a classical callback interface without any dependency to any IPC implemenation as base.

There is no direct mapping to event and msg. The code mapping to event and msg are just example implementations of how to map the callback structure of async sock to RIOT... So what you are proposing is already the case here.

miri64 · 2019-10-08T07:07:42Z

See e.g. 8d33aed for how async sock is handled for sock_udp. If you want some thing more complicated and abstract than just callbacks, we can go back to #8149 but that was stalled for years...

miri64 · 2019-10-08T07:09:22Z

If this is already the case and I just did not read the code of this PR correctly then never mind :-)

Sorry, should have read your comment to the very end 😅. Yes, what you are proposing is already the case.

haukepetersen · 2019-10-08T07:13:02Z

Now I saw, I mainly missed the difference of sock_event_type_t and sock_event_t :-)

miri64 · 2019-10-08T08:17:49Z

I gave this PR some well deserved love:

Rebased to current master
Added DTLS support
Made sure the sock_async_* modules are compilable ;-)

All that's missing is some doc on the sock_async_* modules IMHO and then it might be ready to go (of course the network stacks need to implement the event triggering as well, but that I rather would do as a follow-up).

miri64 · 2019-10-08T11:34:36Z

Added some doc for sock_async_event.

miri64 · 2019-10-08T13:58:12Z

OK. I updated all the doc now. Anybody wants to review. I did not test, if the examples I provide work, as there is no implementation of this API yet, but I'd be happy to provide one for GNRC and lwIP, once I get some positive feedback.

miri64 · 2019-10-08T18:18:49Z

If people prefer it, I can factor out the example front-end implementations into separate PRs.

miri64 · 2019-10-11T13:46:11Z

@haukepetersen @kaspar030 et al. can I please have some feedback on this?

kaspar030 · 2019-10-29T14:34:15Z

sys/net/sock/async/event/sock_async_event.c

+    ctx->event.sock = sock;
+    ctx->event.type |= type;
+    event_post(ctx->queue, &ctx->event.super);
+    ctx->event.type &= ~type;


this must stay set until after the cb has been executed.

miri64 · 2019-10-29T15:39:12Z

@kaspar030 and I discussed a bit about this online and we decided to take out the the front-end implementations for now for several reason:

To get this ahead and simplify the review
It is not clear if sock_async_msg makes sense at the moment
sock_async_event needs improvement
There is potential for a unified event handler that can be re-used by all front-end implementations.

As this needs some cherry-picking and reverting I will go ahead and squash everything.

miri64 · 2019-10-29T15:45:14Z

Squashed first (and renamed the event type flags as they caused some confusion offline)

miri64 · 2019-10-29T15:56:20Z

And squashed and renamed again (forgot to rename the usage of that type)

miri64 · 2019-10-29T15:57:19Z

Aand removed the implementations. Ready for review again.

miri64 · 2019-10-29T16:05:48Z

So they don't get lost, find the implementations in #12601 and #12602. I marked them as demonstrator (core_msg) and WIP (event) respectively, to mark their current state (core_msg not sure if want but provides an example, event needs some work).

miri64 · 2019-10-29T16:16:53Z

Pushed also some doc adaptions to be more in line with the new name.

kaspar030 · 2019-10-30T10:55:27Z

sys/include/net/sock.h

+ * @note    Only applicable with @ref SOCK_HAS_ASYNC defined.
+ */
+typedef enum {
+    SOCK_EVENT_CONN_RDY  = 0x0001,  /**< Connection ready event */


maybe SOCK_ASYNC_foo?

kaspar030 · 2019-10-30T10:56:44Z

sys/include/net/sock/dtls.h

+
+#if SOCK_HAS_ASYNC_CTX || defined(DOXYGEN)
+/**
+ * @brief   Gets the asynchronous


kaspar030

Looks very promising!

We discussed offline that while this API is not stable we'll keep it separate from the existing sock headers and just copy all changes from this PR into a shared sock/async.h.
That way existing code, API and headers stay untouched for now.

miri64 · 2019-10-30T12:24:00Z

We discussed offline that while this API is not stable we'll keep it separate from the existing sock headers and just copy all changes from this PR into a shared sock/async.h.
That way existing code, API and headers stay untouched for now.

Done and squashed (as I removed most commits anyway).

kaspar030

ACK. Great starting point.

miri64 · 2019-10-30T16:12:55Z

Thanks for reviewing and getting this finally in!

miri64 added Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Jun 19, 2019

miri64 requested a review from kaspar030 June 19, 2019 13:03

miri64 added this to the Release 2019.10 milestone Jun 19, 2019

miri64 mentioned this pull request Jun 19, 2019

sock: extend API for asynchronous event management #8149

Closed

This was referenced Aug 10, 2019

gnrc_sock: provide asynchronous event implementation #8236

Closed

posix: missing functionality #7801

Open

miri64 force-pushed the sock/enh/async2 branch from c928fbd to 77534b1 Compare October 8, 2019 08:15

kb2ma modified the milestones: Release 2019.10, Release 2020.01 Oct 9, 2019

kaspar030 reviewed Oct 29, 2019

View reviewed changes

miri64 force-pushed the sock/enh/async2 branch from 3f7c83b to 1930d5b Compare October 29, 2019 15:44

miri64 force-pushed the sock/enh/async2 branch from 1930d5b to 2ffb895 Compare October 29, 2019 15:56

miri64 force-pushed the sock/enh/async2 branch from 2ffb895 to 1281d15 Compare October 29, 2019 15:56

This was referenced Oct 29, 2019

sock_async: initial import of msg-based implementation #12601

Closed

sock_async: initial import of event-based implementation #12602

Merged

kaspar030 reviewed Oct 30, 2019

View reviewed changes

sys/include/net/sock/dtls.h Outdated

#if SOCK_HAS_ASYNC_CTX || defined(DOXYGEN)

/**

* @brief Gets the asynchronous

Copy link

Contributor

kaspar030 Oct 30, 2019

Choose a reason for hiding this comment

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

.

kaspar030 requested changes Oct 30, 2019

View reviewed changes

sock: initial import of asynchronous event definitions

f0b5eb0

miri64 force-pushed the sock/enh/async2 branch from bb596b9 to f0b5eb0 Compare October 30, 2019 12:23

kaspar030 approved these changes Oct 30, 2019

View reviewed changes

miri64 merged commit 5b4b12e into RIOT-OS:master Oct 30, 2019

miri64 deleted the sock/enh/async2 branch October 30, 2019 16:12

This was referenced Oct 31, 2019

sock_async: fix compile-time errors #12624

Merged

gnrc_sock: provide asynchronous event implementation #12625

Merged

miri64 mentioned this pull request Dec 11, 2019

sock_async: fix cyclic include problem #12921

Merged

Conversation

miri64 commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

kaspar030 commented Jun 19, 2019

Uh oh!

miri64 commented Jun 19, 2019

Uh oh!

miri64 commented Jun 19, 2019

Uh oh!

miri64 commented Jun 19, 2019

Uh oh!

haukepetersen commented Oct 8, 2019

Uh oh!

miri64 commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Oct 8, 2019

Uh oh!

miri64 commented Oct 8, 2019

Uh oh!

haukepetersen commented Oct 8, 2019

Uh oh!

miri64 commented Oct 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Oct 8, 2019

Uh oh!

miri64 commented Oct 8, 2019

Uh oh!

miri64 commented Oct 8, 2019

Uh oh!

miri64 commented Oct 11, 2019

Uh oh!

kaspar030 Oct 29, 2019

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

miri64 commented Oct 29, 2019

Uh oh!

kaspar030 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kaspar030 Oct 30, 2019

Choose a reason for hiding this comment

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 30, 2019

Uh oh!

kaspar030 left a comment

Choose a reason for hiding this comment

Uh oh!

miri64 commented Oct 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miri64 commented Jun 19, 2019 •

edited

Loading

miri64 commented Oct 8, 2019 •

edited

Loading

miri64 commented Oct 8, 2019 •

edited

Loading