Initial version of tox_loop, the low-latency API

[cleverca22]

this PR adds a tox_loop() function that will block on the network sockets and call tox_iterate automatically

along with 2 callbacks to allow a client to manage a mutex to maintain thread safety

---

This change is

[iphydf]

Review status: 0 of 8 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

auto_tests/tox_loop_test.c, line 4 at r1 (raw file):

#include <stdio.h>
#include <stdlib.h>
#include <pthread.h>

Sort headers. Reverse the blocks (system headers last).

---

other/apidsl/tox.in.h, line 815 at r1 (raw file):

/**
 * run tox_iterate automaticaly, only returns after tox_loop_stop()

Capitalise and use punctuation. Also use $iterate and ${loop.stop} references.

---

other/apidsl/tox.in.h, line 823 at r1 (raw file):

   * tell tox_loop to return
   */
  void stop();

Add empty lines between each declaration in namespace loop.

---

other/apidsl/tox.in.h, line 834 at r1 (raw file):

  }
  /**
   * event ran with tox_loop is finished with Tox*

This sentence does not parse. Please revise all the comments in your additions. Take a look at other events for inspiration.

---

toxcore/TCP_connection.c, line 1442 at r1 (raw file):

    free(tcp_c);
}
int tcp_collect_fds(TCP_Connections *tcp_c, uint32_t *sockets, uint32_t max_sockets)

Empty line before definition.

---

toxcore/TCP_connection.c, line 1448 at r1 (raw file):

    for (x = 0; x < tcp_c->tcp_connections_length; x++) {
        if (max_sockets == 0) break;

Braces and newlines. Add to all the single line ifs you have.

---

toxcore/tox.c, line 519 at r1 (raw file):

    struct timeval timeout;
    int maxfd;
    uint32_t fdlist[20], i;

Where does this arbitrary limit of 20 come from?

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 8 files at r1.
Review status: 1 of 8 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

auto_tests/tox_loop_test.c, line 43 at r1 (raw file):
Check Travis builds, e.g.

/home/travis/build/TokTok/toxcore/auto_tests/tox_loop_test.c:44:1: warning:
control reaches end of non-void function [-Wreturn-type]

---

Comments from Reviewable

[nurupo]

@cleverca22 could explain why this PR is needed and why we should accept it? I know there was an IRC conversation, but it's nice to have this explained for the record and I didn't follow the conversation that much.

[nurupo]

I gave it just a quick glance. I will review this more thoughtfully later.

[cleverca22]

if tox_iterate_interval() returns 50ms, and you only ever run tox_iterate() every 50ms, then it cant process received packets any more often

this leads to things like 50ms ping to a toxcore node on the same LAN, and 200ms pings to a toxcore node that only gets under ~57ms outside of tox

letting toxcore block on a select() call over all network FD's allows it to instantly respond to any incoming packets

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions.

---

other/apidsl/tox.in.h, line 823 at r1 (raw file):

Previously, iphydf wrote…

Add empty lines between each declaration in namespace loop.

A comment is not a declaration. Remove the empty line after the comment.

---

other/apidsl/tox.in.h, line 831 at r2 (raw file):

  event start const {
    /**
     * callback ran when tox_loop calls into tox_iterate, the client can lock a mutex here

Capitalisation, punctuation, references.

---

other/apidsl/tox.in.h, line 839 at r2 (raw file):

   * Event ran when $loop is finished with Tox*.
   */

Remove newline.

---

other/apidsl/tox.in.h, line 842 at r2 (raw file):

  event stop const {
    /**
     * callback ran when tox_loop is finished with tox_iterate, the client can unlock the mutex here

Capitalisation, punctuation, references. Also, this documentation should be on the event itself. The typedef usually just gets parameter docs. Since there are no params here, you could document the fact that this event receives no data and just its invocation is all the information you need.

---

Comments from Reviewable

[cleverca22]

updated

[cleverca22]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions.

---

auto_tests/tox_loop_test.c, line 4 at r1 (raw file):

Previously, iphydf wrote…

Sort headers. Reverse the blocks (system headers last).

Done.

---

auto_tests/tox_loop_test.c, line 43 at r1 (raw file):

Previously, nurupo wrote…

Check Travis builds, e.g.

/home/travis/build/TokTok/toxcore/auto_tests/tox_loop_test.c:44:1: warning:
control reaches end of non-void function [-Wreturn-type]

Done.

---

other/apidsl/tox.in.h, line 815 at r1 (raw file):

Previously, iphydf wrote…

Capitalise and use punctuation. Also use $iterate and ${loop.stop} references.

Done.

---

other/apidsl/tox.in.h, line 834 at r1 (raw file):

Previously, iphydf wrote…

This sentence does not parse. Please revise all the comments in your additions. Take a look at other events for inspiration.

Done.

---

other/apidsl/tox.in.h, line 831 at r2 (raw file):

Previously, iphydf wrote…

Capitalisation, punctuation, references.

Done.

---

other/apidsl/tox.in.h, line 839 at r2 (raw file):

Previously, iphydf wrote…

Remove newline.

Done.

---

other/apidsl/tox.in.h, line 842 at r2 (raw file):

Previously, iphydf wrote…

Capitalisation, punctuation, references. Also, this documentation should be on the event itself. The typedef usually just gets parameter docs. Since there are no params here, you could document the fact that this event receives no data and just its invocation is all the information you need.

Done.

---

toxcore/tox.c, line 519 at r1 (raw file):

Previously, iphydf wrote…

Where does this arbitrary limit of 20 come from?

Done.

---

toxcore/TCP_connection.c, line 1442 at r1 (raw file):

Previously, iphydf wrote…

Empty line before definition.

Done.

---

toxcore/TCP_connection.c, line 1448 at r1 (raw file):

Previously, iphydf wrote…

Braces and newlines. Add to all the single line ifs you have.

Done.

---

Comments from Reviewable

[cleverca22]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions.

---

other/apidsl/tox.in.h, line 823 at r1 (raw file):

Previously, iphydf wrote…

A comment is not a declaration. Remove the empty line after the comment.

Done.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 22 unresolved discussions.

---

auto_tests/tox_loop_test.c, line 64 at r3 (raw file):

    pthread_create(&worker1, NULL, tox_loop_worker, &userdata);

Perhaps replace the double empty lines with a single empty line plus a comment what the block of code does.

---

auto_tests/tox_loop_test.c, line 108 at r3 (raw file):

static Suite *tox_suite(void)
{
    Suite *s = suite_create("Tox");

"Tox loop"?

---

other/apidsl/tox.in.h, line 815 at r3 (raw file):

/**
 * Run $iterate() automaticaly, only returns after ${loop.stop}().

"automatically". But this comment doesn't really explain what the function does. Running a function automatically could also just mean that it invokes it, and then sleeps until loop.stop().

---

other/apidsl/tox.in.h, line 823 at r3 (raw file):

   * Tell $loop() to return.
   */

Remove empty line (again).

---

toxcore/tox.c, line 484 at r3 (raw file):

/**
 * Return the total FD count.

This comment is next to useless. tox_fd_count quite clearly states that it returns the fd count. Either elaborate or remove the comment.

---

toxcore/tox.c, line 491 at r3 (raw file):

    return 1 + m->net_crypto->tcp_c->tcp_connections_length;
}
/**

Add newline before comment.

---

toxcore/tox.c, line 492 at r3 (raw file):

}
/**
 * Gathers a list of every network FD to wait on.

It gathers a list of every FD that we expect network activity on. Whether the client code waits on it or polls is not up to this function.

---

toxcore/tox.c, line 494 at r3 (raw file):

 * Gathers a list of every network FD to wait on.
 */
uint32_t tox_fds(Tox *tox, uint32_t *sockets, uint32_t max_sockets)

I'd prefer you just require (and document the requirement) that sockets is an array of length tox_fd_count. This is how the rest of the public API works as well. See e.g. the top level comment in tox.h about "Threading implications", and function names ending in _size.

---

toxcore/tox.c, line 545 at r3 (raw file):

    if (m->loop_start_cb) {
        m->loop_start_cb(tox, user_data);

Can we call this at the beginning of the loop, then immediately call tox_iterate (so tox_iterate will be called before select), and call the stop cb at the end of the loop?

---

toxcore/tox.c, line 564 at r3 (raw file):

            FD_SET(fdlist[i], &readable);

            if (fdlist[i] > maxfd) maxfd = fdlist[i];

Braces.

---

toxcore/tox.c, line 568 at r3 (raw file):

        timeout.tv_sec = 0;
        timeout.tv_usec = tox_iteration_interval(tox) * 1000 * 2; // TODO, use a longer timeout

Add either a bug number or github username in parentheses after TODO. See #133.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

auto_tests/tox_loop_test.c, line 43 at r1 (raw file):

Previously, cleverca22 wrote…

Done.

tox_loop returns `uint32_t`. tox_loop_worker returns `void*`. While this is valid C code, I prefer an explicit cast if you intend to use the return value. If not, return NULL.

---

auto_tests/tox_loop_test.c, line 76 at r4 (raw file):

    tox_callback_loop_start(userdata_tcp.tox, tox_loop_cb_start);
    tox_callback_loop_stop(userdata_tcp.tox, tox_loop_cb_stop);
    pthread_create(&worker2, NULL, tox_loop_worker, &userdata_tcp);

These threads are never joined. We don't know whether they actually return or keep running forever (until process termination).

---

Comments from Reviewable

[cleverca22]

Review status: 1 of 6 files reviewed at latest revision, 12 unresolved discussions.

---

auto_tests/tox_loop_test.c, line 43 at r1 (raw file):

Previously, iphydf wrote…

tox_loop returns uint32_t. tox_loop_worker returns void*. While this is valid C code, I prefer an explicit cast if you intend to use the return value. If not, return NULL.

Done.

---

auto_tests/tox_loop_test.c, line 64 at r3 (raw file):

Previously, iphydf wrote…

Perhaps replace the double empty lines with a single empty line plus a comment what the block of code does.

Done.

---

auto_tests/tox_loop_test.c, line 108 at r3 (raw file):

Previously, iphydf wrote…

"Tox loop"?

Done.

---

auto_tests/tox_loop_test.c, line 76 at r4 (raw file):

Previously, iphydf wrote…

These threads are never joined. We don't know whether they actually return or keep running forever (until process termination).

Done.

---

other/apidsl/tox.in.h, line 815 at r3 (raw file):

Previously, iphydf wrote…

"automatically". But this comment doesn't really explain what the function does. Running a function automatically could also just mean that it invokes it, and then sleeps until loop.stop().

Done.

---

toxcore/tox.c, line 484 at r3 (raw file):

Previously, iphydf wrote…

This comment is next to useless. tox_fd_count quite clearly states that it returns the fd count. Either elaborate or remove the comment.

Done.

---

toxcore/tox.c, line 491 at r3 (raw file):

Previously, iphydf wrote…

Add newline before comment.

Done.

---

toxcore/tox.c, line 492 at r3 (raw file):

Previously, iphydf wrote…

It gathers a list of every FD that we expect network activity on. Whether the client code waits on it or polls is not up to this function.

Done.

---

toxcore/tox.c, line 545 at r3 (raw file):

Previously, iphydf wrote…

Can we call this at the beginning of the loop, then immediately call tox_iterate (so tox_iterate will be called before select), and call the stop cb at the end of the loop?

Done.

---

toxcore/tox.c, line 564 at r3 (raw file):

Previously, iphydf wrote…

Braces.

Done.

---

Comments from Reviewable

[cleverca22]

Review status: 1 of 6 files reviewed at latest revision, 12 unresolved discussions.

---

toxcore/tox.c, line 494 at r3 (raw file):

Previously, iphydf wrote…

I'd prefer you just require (and document the requirement) that sockets is an array of length tox_fd_count. This is how the rest of the public API works as well. See e.g. the top level comment in tox.h about "Threading implications", and function names ending in _size.

Done.

---

toxcore/tox.c, line 568 at r3 (raw file):

Previously, iphydf wrote…

Add either a bug number or github username in parentheses after TODO. See #133.

Done.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 17 unresolved discussions.

---

auto_tests/tox_loop_test.c, line 43 at r1 (raw file):

Previously, cleverca22 wrote…

Done.

Replace `int` with `uint32_t`.

Alternatively, write the result to the loop_test data? Casting between integers and pointers is valid on most platforms, but there is really no need for it in here, so you can simply avoid the confusion and non-portable code.

---

auto_tests/tox_loop_test.c, line 64 at r3 (raw file):

Previously, cleverca22 wrote…

Done.

I don't see any comment. Am I missing something?

---

auto_tests/tox_loop_test.c, line 51 at r5 (raw file):

START_TEST(test_tox_loop)
{
    pthread_t worker1, worker2;

Rename in a way that makes it clear which worker maps to which userdata. Right now it's worker1 -> userdata, worker2 -> userdata_tcp.

---

toxcore/Messenger.h, line 270 at r5 (raw file):

    unsigned int last_connection_status;

    uint8_t loop_run;

bool or _Bool (for now we don't have stdbool.h, yet, but we'll have it soon).

---

toxcore/tox.c, line 535 at r5 (raw file):

uint32_t tox_loop(Tox *tox, void *user_data)
{
    struct timeval timeout;

Reduce scope of declarations as much as possible. Move them to their first assignment if possible (but don't use C99 for-init-decls).

---

toxcore/tox.c, line 539 at r5 (raw file):

    uint32_t i, list_size = 0;
    uint32_t *fdlist = NULL;
    Messenger *m = tox;

Make this the first statement in the function.

---

toxcore/tox.c, line 581 at r5 (raw file):

        if (ret < 0) {
            return ret;

This error handling is not documented in the API. It also deviates from error handling done in all other functions. If you want to propagate an error condition to the caller, use error enums.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 17 unresolved discussions.

---

toxcore/tox.c, line 568 at r3 (raw file):

Previously, cleverca22 wrote…

Done.

Ok. Did you see #133? The format is `TODO(user|bug): ...`.

---

Comments from Reviewable

[iphydf]

Review status: 1 of 6 files reviewed at latest revision, 10 unresolved discussions.

---

toxcore/tox.c, line 492 at r3 (raw file):

Previously, cleverca22 wrote…

Done.

Add punctuation to form a sentence.

---

toxcore/tox.c, line 494 at r3 (raw file):

Previously, cleverca22 wrote…

Done.

Not really. Now it only documents the requirement but doesn't actually require it. If you prefer a defence in depth approach here, and in addition to requiring the caller to have an appropriately sized array, you also require them to tell you about it, it would make sense to return error and do nothing if the size doesn't match. This error would then need to be handled in the caller. If you don't want the defence in depth, you can get rid of `max_sockets`.

---

Comments from Reviewable

[JFreegman]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

toxcore/tox.c, line 557 at r5 (raw file):

        if (fdcount > list_size) {
            fdlist = realloc(fdlist, fdcount * sizeof(uint32_t));

This realloc() call needs error handling, and fdlist needs to be free'd before the function exits.

---

toxcore/tox.c, line 567 at r5 (raw file):

            if (fdlist[i] > maxfd) {
                maxfd = fdlist[i];

Assigning a uint32_t to an int is a bad idea. Same goes for the implicit cast with FD_SET(). Probably not a pragmatic concern here but it's bad form. Why isn't fdlist an int array?

---

Comments from Reviewable

[cleverca22]

Review status: 1 of 6 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

---

toxcore/tox.c, line 567 at r5 (raw file):

Previously, JFreegman wrote…

Assigning a uint32_t to an int is a bad idea. Same goes for the implicit cast with FD_SET(). Probably not a pragmatic concern here but it's bad form. Why isn't fdlist an int array?

the original patch (before the PR) was exposing the FD over the public api, and apisdl refused to expose an int

---

Comments from Reviewable

[GrayHatter]

Reviewed 1 of 8 files at r1.
Review status: 1 of 6 files reviewed at latest revision, 13 unresolved discussions, some commit checks failed.

---

a discussion (no related file):
Do we really want this much logic in tox.c?

IMO this should be done a lot closer to net_crpyto or network even.

Why did you choose tox.c and messenger.h for the logic/data

---

other/apidsl/tox.in.h, line 826 at r5 (raw file):

  /**
   * Callback ran when $loop() calls into $iterate(), the client can lock a mutex here.

I Suggest:

This callback is invoked when....

---

Comments from Reviewable

[iphydf]

What's the progress on this? Can you rebase and fix the conflicts?

[cleverca22]

need to set aside some time to fix all of the things commented on and rebase

[vassad]

Hey! How is it going? It seems to be a very important core update

[iphydf]

@vassad: this has been taken on by @Ansa89 in #335. I'm closing this one in favour of that one. @cleverca22 can push commits to @Ansa89's branch.

[Ansa89]

@vassad: this has been taken on by @Diadlo in #335. I'm closing this one in favour of that one.
@cleverca22 can push commits to @Diadlo's branch.

I thought #335 was mine...Am I missing something?

[Diadlo]

@iphydf I skipped you post in the first time. But for some reason I'm assigned to #225. Is this correct?)

[iphydf]

Right, I meant @Ansa89. Sorry about that.