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

Documentation for timeout in enet_host_service is unclear #136

Open
JayFoxRox opened this issue Aug 25, 2020 · 1 comment
Open

Documentation for timeout in enet_host_service is unclear #136

JayFoxRox opened this issue Aug 25, 2020 · 1 comment

Comments

@JayFoxRox
Copy link

I'm not actively using enet at the moment. But skimming over some of the issues and the API have made me think about some potential usage issues which are a bit ambigiously documented / undocumented.


The first problem is related to the timeout value being used in a loop, so a timeout of 1000 might not just wait up to 1000ms, but much longer:

enet/docs/tutorial.dox

Lines 166 to 167 in 2e1c6bc

/* Wait up to 1000 milliseconds for an event. */
while (enet_host_service (client, & event, 1000) > 0)

This probably isn't actually true, for reasons described in the final list of #135 (comment): While it will wait up to 1000ms for each event, the entire loop (which the comment could be about) could take much longer if new events keep coming.

It would be interesting to know if it might be possible to use a code pattern like this (I'm not sure if enet works like this):

// The first call to `enet_host_service` will wait up to 1000ms if there's nothing to do
unsigned int timeout = 1000;
while(enet_host_service (client, &event, timeout) > 0) {
    handle_event(&event);

    // If there was something to do, try to handle events as quickly as possible
    timeout = 0;

    // Question: If an event happens after 700ms, will the next call to `enet_host_service` still wait
    //           the remaining 300ms, even if the timeout is 0 now?
    //           = Does the "unused" timeout time accumulate?
}

To me, the following implies the timeout time accumulation behavior (where one could use enet_host_service(client, NULL, 2000) to "allocate" 2000ms until enet_host_service would actually be allowed to return without event, even if a timeout of 0 is used next time).

enet/protocol.c

Lines 1778 to 1780 in 2e1c6bc

@param event an event structure where event details will be placed if one occurs
if event == NULL then no events will be delivered
@param timeout number of milliseconds that ENet should wait for events

(Implied timeout accumulation because of "wait for events", even though this function returns a single event. Also implied by ability to check for events without delivering them)


This also raises the question: are non-delivered events (where event == NULL) discarded or simply deferred until a call to enet_host_service without event == NULL?
Also will a call with event == NULL and timeout > 0 still be blocking?


Another potential issue I mention in #135 (comment) is network flooding, where packets arrive more quickly than the enet_host_service loop can handle.

So even if the user is aware that the timeout might be longer than what they intended, then they might still not realize that the loop could be infinitely long. Even if the timeout is 0 this can be an issue if processing each event takes too long.

That might be something to address in the documentation.
In that case, the loop should also have a iteration limit like this:

// Handle 2000 events at most, to avoid hanging if someone floods the network
unsigned int iteration_limit = 2000;
unsigned int timeout = 0;
while(iteration_limit-- && enet_host_service (client, &event, timeout) > 0) {
    slow_processing(); // During this, new packets might have already arrived, requiring another iteration
    [...]
}

// Now that network is done, do other critical tasks
// Caveat: This might never be reached if we don't have the iteration_limit
@imtrobin
Copy link

I agree with the possible problem without iteration_limit , but I'm not sure if it's needed in practice, becos if you can't process faster than network is sending, then it's likely a ddos.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants