Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add udp backend #76

Closed
wants to merge 8 commits into from
Closed

Add udp backend #76

wants to merge 8 commits into from

Conversation

jakobod
Copy link
Member

@jakobod jakobod commented Jun 16, 2020

This PR adds a simple udp backend and a datagram_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_transport implementation including the transport_worker_dispatcher.

This PR is based on #71 and should be merged/reviewed after that has been merged.
closes #54

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)))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can understand that. I'll remove the duplicate port field when I'm fixing the other remark.

@jakobod jakobod closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Datagram layer for applications
2 participants