Skip to content

sock: extend API for asynchronous event management#8149

Closed
miri64 wants to merge 7 commits intoRIOT-OS:masterfrom
miri64:sock/enh/async
Closed

sock: extend API for asynchronous event management#8149
miri64 wants to merge 7 commits intoRIOT-OS:masterfrom
miri64:sock/enh/async

Conversation

@miri64
Copy link
Member

@miri64 miri64 commented Nov 27, 2017

This is a first draft to extend the sock APIs for asynchronous events using the event module.

I kept it optionally to

  1. be backwards compatible for older sock implementations
  2. be compatible to non-RIOT implementations, like @kaspar030's wrapper around POSIX sockets.

Doc and implementation will come as soon as I get the go from enough people.

@miri64 miri64 added Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Nov 27, 2017
@miri64
Copy link
Member Author

miri64 commented Nov 27, 2017

Dead-simple alternative could be (like is AFAIK planed with saul) to just add callbacks instead. This was however declined due to concerns about the call context of that callback [edit](while with saul it is clear that it will always come from the ISR)[/edit] in earlier versions of sock. This is why I went for event instead.

@kaspar030
Copy link
Contributor

This is why I went for event instead.

Nice. I always had sock in mind when doing the events.

Maybe we can tackle two things in one API rework. I think it is time to have a generic sock type which allows application logic to be written (mostly) independently from the underlying transport.
That generic struct would be the ideal place to add async support.

(I'm being inspired by dsock).

sth like:

typedef struct {
read, write, close, get/set, ...
} sock_ops_t;
typedef struct {
  sock_ops_t ops;
} sock_t;

typedef struct {
    sock_t super;
...
} sock_tcp_t;

static inline sock_read(sock_t *sock, ...) {
    sock->ops->read(sock, ...);
}
...

Another very useful addition would be scatter/gather IO to allow easier stacking of protocols (e.g., websocket on https on tcp. smile please.).

Something like:

struct iolist {
  struct iolist *next;
  void *buf;
  size_t buf_len;
};

sock_readv(sock, struct iolist *list);

That way, if a very-high level protocol (e.g., json-rpc over websocket over http over tcp) provides a "struct iolist" with just one entry, every protocol below in the stack can extend that list (adding data before or after the user data), without having to re-allocate a sufficiently sized buffer for handing down to the next lower protocol in the stack. (struct iovec doesn't cut it as the array would have to be allocated at the highest level, which doesn't know the needed size).

@miri64
Copy link
Member Author

miri64 commented Dec 1, 2017

Nice. I always had sock in mind when doing the events.

I know, I just wanted to convince some people other than you ;).

Regarding the generic sock: I'll read your whole comment when I'm back on Monday, but for now I just wanted to say that I was also thinking of something like this (more from a lower-level point however, due to stuff like coap over UART or coap over SMS). I envisioned it more as an API on top of sock (and other APIs) instead of an API rework. As such, I would rather further discuss this in a seperate PR or (since I also think we need to discuss this a little first) issue.

@miri64
Copy link
Member Author

miri64 commented Dec 1, 2017

(also to intercept possible derailment of the discussion on asynchronouos sock).

@miri64
Copy link
Member Author

miri64 commented Dec 3, 2017

added some doc

@miri64
Copy link
Member Author

miri64 commented Dec 11, 2017

Rebased and simplified API a bit, after some offline discussion with @kaspar030.

@miri64
Copy link
Member Author

miri64 commented Dec 11, 2017

I provided an implementation for GNRC in #8236. It is only ~100 lines of code :-).

@miri64 miri64 added State: waiting for other PR State: The PR requires another PR to be merged first and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Feb 14, 2018
@miri64
Copy link
Member Author

miri64 commented Feb 14, 2018

@kaspar030 can we maybe discuss your proposal for a unified sock in a separate issue and for now go on with this? It would help to make sock more stack independent (see #8130).

@miri64
Copy link
Member Author

miri64 commented Feb 27, 2018

Ping? Can maybe someone review this at today's Hack'n'ACK? This might be useful for the DHCPv6 implementation I plan to champion at the next IETF hackathon (mid of March).

@kaspar030
Copy link
Contributor

Maybe let's go through the API F2F next week?

@miri64
Copy link
Member Author

miri64 commented Mar 6, 2018

Ok, result of the F2F discussion: Implementation looks good according to @kaspar030, but the API is currently

  1. too RIOT-specific due to the usage of event.h (sock is supposed to be portable after all) and
  2. too code-duplication-prone. The idea is to provide a generic sock on top of the specific sock APIs and let that be the API that provides the handlers.

@kb2ma
Copy link
Member

kb2ma commented Mar 6, 2018

I just worked through how to integrate the code in this PR and #8236 with gcoap. It's definitely nice to have the event abstraction and timeout handling! Given your meeting, I'll hold off on further work until you've got something to share.

@miri64
Copy link
Member Author

miri64 commented Mar 6, 2018

Good APIs are like good wine, the longer they age the better they get :P.

@tcschmidt
Copy link
Member

Good APIs are like good wine, the longer they age the better they get :P.
@miri64 better wait for another season?

@kaspar030 How can a RIOT API be too RIOT-specific?

@miri64
Copy link
Member Author

miri64 commented Jun 20, 2018

@kaspar030 How can a RIOT API be too RIOT-specific?

sock was originally designed to also run on Linux, so I wouldn't say, sock is a RIOT API.

However, since there is again some back-chatter that implies the urgency of this matter (the initially mentioned gCoAP, the asynchronous MQTT-SN implementation by @haukepetersen, @rfuentess's inclusion of this PR into #9066, ...) and we did not look further into a "new sock" yet (at least publicly?), I'd like to put this PR again back on the table. For the an overview of the counter-arguments, please have a look at #8149 (comment).

There are currently a few ideas floating around to put some traction into this:

  • keep the current proposal and break portability of sock (or make port event.h also portable).
  • use callbacks instead of events with the explicit restriction that these are not to be used for further work on the data, but is supposed to hand the data over to a proper handler (similar to how we do it with ISR handlers currently).
  • Find some other way of (portable) event handling (e.g. a sock-specific event library, something else more complicated, magic?, etc.)

@tcschmidt
Copy link
Member

@kaspar030 How can a RIOT API be too RIOT-specific?

sock was originally designed to also run on Linux, so I wouldn't say, sock is a RIOT API.

What rationale is behind this? I don't see any use case for RIOT system APIs to be brought into the Linux community ... or are Linux guys urgently lacking system interfaces?

@miri64 will try to look into the remaining discussion asap.

@miri64
Copy link
Member Author

miri64 commented Jun 20, 2018

What rationale is behind this? I don't see any use case for RIOT system APIs to be brought into the Linux community ... or are Linux guys urgently lacking system interfaces?

Again, sock was not designed as a RIOT system library, but as an alternative to POSIX sockets. If a developer want's to use them instead of sockets (which are kind of demanding compared to sock) a developer can do this using this library. The rational behind this is portability. We even mentioned this in the RIOT paper.

@miri64
Copy link
Member Author

miri64 commented Jun 20, 2018

@miri64 will try to look into the remaining discussion asap.

I also asked @bergzand as a fresh pair of eyes to do so offline.

@tcschmidt
Copy link
Member

a developer can do this using this library. The rational behind this is portability. We even mentioned this in the RIOT paper.

@miri64 I'm not saying you're wrong, I'm questioning the sense of the approach from a RIOT perspective.

RIOT paper says:
"Conversely, sock can easily be wrapped around a POSIX socket, thus making applications developed against sock portable to any POSIX-based operating system"

Which is short, but weird. POSIX is the portability standard for system APIs, not RIOT internal APIs are the portability approach to the remaining world. Of course you can always wrap things around each other and if @kaspar030 has private hobbies, that's fine.

From a RIOT design perspective, it IMO makes absolutely no sense to design system APIs so that we can promote them to other OSes. Instead, technical feasibility and appropriate usefulness in the RIOT IoT context should solely guide our design.

@tcschmidt
Copy link
Member

I kept it optionally to

  1. be backwards compatible for older sock implementations
  2. be compatible to non-RIOT implementations, like @kaspar030's wrapper around POSIX sockets.

That is an easy choice: backward compatibility, since 2. is a crystal-clear NON-goal of the RIOT community.

Changing sock (the RIOT networking API) again is opening Pandora's box and will feed the already well established reputation that RIOT is unusable, because its APIs change permanently, randomly, and without rational reasons.

@bergzand
Copy link
Member

In my opinion the ability to develop a network application on a Linux based system and being able to deploy it without modifications on RIOT is quite useful and IMHO something that could be advertised a bit more. I found it rather nice to do development on Nanocoap without having to take the RIOT environment into account, but still having some area cost guarantees as soon as it's used in RIOT.

too RIOT-specific due to the usage of event.h (sock is supposed to be portable after all) and

Would it be possible to do something as is currently done with sock_types.h and wrap all the (in this case event.h) handling to something generic for sock? I'm trying to get an estimate the impact and viability of this.

@tcschmidt
Copy link
Member

In my opinion the ability to develop a network application on a Linux based system and being able to deploy it without modifications on RIOT is quite useful and IMHO something that could be advertised a bit more.

Perfectly agreed - that's what API standards are good for: Portability between OSes, e.g. by POSIX. And this is why we aim at POSIX compliance since the first strategy layout of RIOT in 2012.
Strangely, in this discussion we are inverting the perspective and try to bring a RIOT compatibility layer to Linux ???

@miri64
Copy link
Member Author

miri64 commented Jun 22, 2018

Another thing to consider, why the current approach might not be the most ideal, is that it basically fixes the user to use event.h for their event handling. However, what if their application uses a different (RIOT-specific) way of handling events (e.g. thread_flags, core_msg, core_mbox, callbacks, ...)? The only way to handle events than would be to somehow find the least common denominator or write a wrapper around the other event handling methods using event.h. That's a) not very nice to the user b) generates more (code and time) overhead.

@tcschmidt
Copy link
Member

@miri64 fine with me: the important message was that sock will not be changed (only extended) which we agreed on in today's meeting.

@jcarrano
Copy link
Contributor

jcarrano commented Jul 6, 2018

My understanding of this API is: You set a fixed handler and a fixed event queue for each socket and then when an "something happens" in that socket, an event gets sent to the event queue with the preset callback.

How is this different to just setting a completion/reception callback (other than being able to run the callback on a different context)? Why bother with the event queue then? To me it seems as an under-utilization of the event queue capabilities.

  • Do we want an event source per-socket (like in Linux) or per operation (like IOCP in Windows)? Some implications:
    • Linux style: when doing non-blocking calls there is no need to identify the operation.
    • Windows style: Each operation has some structure/handle associated. I would say that the current event code lends itself more to IOCP-like operation.
  • Is it necessary / desirable to fix the callback?
  • Is it even desirable to ALWAYS have a callback? This is a question more related to the event queue itself: ¿Why we have the event queueing logic merged with the callback dispatch logic?
  • The events are "sender allocated". When do the sock events get deallocated. Who does it? Answer: the event structure lives with the sock object.

@jcarrano
Copy link
Contributor

jcarrano commented Jul 9, 2018

Another question: how will this behave if there is an event already queued for a socket and while the event is still not handled more data arrives?

@miri64
Copy link
Member Author

miri64 commented Jul 9, 2018

Can we wait for my rework before we discuss this. I think most of your concerns can be addressed via the abstraction.

@jcarrano
Copy link
Contributor

jcarrano commented Jul 9, 2018

@miri64 Sure. Anyways, I think the points I raised are equally valid for any subsystem that wants to implement an asynchronous interface (e.g async-UART, async-SAUL, etc)

@miri64
Copy link
Member Author

miri64 commented Jul 9, 2018

Yes, that's also why I think this discussion belongs somewhere else ;-). Want to open an issue?

@rfuentess rfuentess mentioned this pull request Aug 30, 2018
5 tasks
@kb2ma kb2ma mentioned this pull request Feb 5, 2019
8 tasks
@miri64
Copy link
Member Author

miri64 commented Apr 12, 2019

Ok, I just had an idea how to get this along a bit faster... Let's just KISS and have callbacks and clearly document that they are not supposed to be used without some kind of event management on top (which we can provide for the various ways to handle events in RIOT [msg, mbox, event, ...] in follow-up PRs).

So we would would just add different event type definitions, a callback type, and a callback setter for each sock-type, e.g.

typedef enum {
    SOCK_EVENT_CONN_RDY = 0x0001,
    SOCK_EVENT_CONN_FIN = 0x0002,
    SOCK_EVENT_CONN_RCV = 0x0004,
    SOCK_EVENT_MSG_RCV = 0x0010,
    SOCK_EVENT_MSG_SNT = 0x0020,
    SOCK_EVENT_PATH_PROP = 0x0040,
} sock_event_type_t;

(ref: https://tools.ietf.org/html/draft-ietf-taps-arch-03#section-4.1.5)

void (*sock_udp_cb_t)(sock_udp_t *sock, sock_event_type_t type);
void sock_udp_set_cb(sock_udp_t *sock, sock_udp_cb_t cb);

@miri64
Copy link
Member Author

miri64 commented Jun 19, 2019

I proposed this alternative approach in #11723.

@miri64
Copy link
Member Author

miri64 commented Sep 10, 2019

Again: alternative exists in #11723. Don't know, why I did not close it then.

@miri64 miri64 closed this Sep 10, 2019
@miri64 miri64 deleted the sock/enh/async branch September 10, 2019 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants