Add udp backend #76
Add udp backend #76
Conversation
jakobod
force-pushed
the
topic/datagram-adaptor
branch
from
5d54769
to
a0d726f
Compare
| template <class Parent> | ||
| error handle_data(Parent& parent, span<const byte> received) { | ||
| if (auto err = application_.handle_data( | ||
| parent, make_span(received.data(), basp::header_size))) |
There was a problem hiding this comment.
This hardwires the datagram_adaptor to BASP. Both in terms of constants and in terms of processing logic (header, then payload).
If you see this as a one-shot class to fit BASP into a datagram manager, then we should rename it. If we want to generalize this adaptor abstraction, we should also rethink the interface to the application. We could split handle_data into handle_header and handle_payload for example. Alternatively, we always call handle_data on the application and it dispatches internally (more flexible). In any case we'd also need to propagate how much of the buffer was consumed (if any). Not all protocols have fixed sizes. HTTP, for example, has to read the header until hitting \r\n\r\n. I was already running into this exact problem in another branch I was working on, so maybe should sort that out first? If we'd go with that, the adaptor would simply call handle_data until the application either has consumed the entire datagram or discarded some of it.
There was a problem hiding this comment.
TBH I think with the proposal of #81 the datagram_adaptor would be obsolete. We should discuss that issue first before renaming or rethinking the datagram_adaptor for BASP.
| @@ -36,4 +36,7 @@ CAF_NET_EXPORT extern const size_t max_header_buffers; | |||
| /// Port to listen on for tcp. | |||
| CAF_NET_EXPORT extern const uint16_t tcp_port; | |||
|
|
|||
| /// Port to listen on for udp. | |||
| CAF_NET_EXPORT extern const uint16_t udp_port; | |||
There was a problem hiding this comment.
Do we need two constants? I assume this is for exposing BASP. Since we're going to use the locator (URI) as identifier for the CAF node, we must use either TCP or UDP. In other words, one default port would suffice.
There was a problem hiding this comment.
I thought that BASP could be used with udp or tcp at the same time, thus both transports require different ports to be accessible from the network.
There was a problem hiding this comment.
In the new model, the node ID is the locator in order to get rid of the pseudo-DNS CAF is currently doing. This means there must be exactly one valid way to connect to a BASP instance.
There was a problem hiding this comment.
I forgot about the fixed node ID.. Still I am not sure if that is the best way to solve this problem. Is it really necessary to allow only one possible way to reach a node via BASP?
There was a problem hiding this comment.
By definition, a node can only have a single ID. If we allow multiple IDs pointing to the same BASP, we have the exact same issues we wanted to get rid of with the redesign that @josephnoir initiated.
We could of course offer a "connect there and see who's responding" function, but it would convolute the design quite a bit. Because then, we can't use the ID any longer to identify connections. I honestly don't see an upside here that would be worth the troubles in the first place.
There was a problem hiding this comment.
I can understand that. I'll remove the duplicate port field when I'm fixing the other remark.
This PR adds a simple
udp backendand adatagram_adaptor, which splits received datagrams into header and payload which can then be passed separately to the following application.Also included are some minor bugfixes in the
datagram_transportimplementation including thetransport_worker_dispatcher.This PR is based on #71 and should be merged/reviewed after that has been merged.
closes #54