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

The select function times out too early (IDFGH-5807) #7514

Closed
Emill opened this issue Sep 3, 2021 · 6 comments
Closed

The select function times out too early (IDFGH-5807) #7514

Emill opened this issue Sep 3, 2021 · 6 comments
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@Emill
Copy link

Emill commented Sep 3, 2021

The select function seems to round down the supplied timeout value to the granularity supported by the system. The POSIX standard says that the value should be rounded up instead. This makes timeouts time out too early with esp-idf. For example if you want to do something every 9 ms and the system's tick rate is set to 10 ms, then your loop will become a busyloop since 9 is rounded down to 0, making select return immediately.

Similar issue for pthread_cond_timedwait: #6901.

In components/vfs/vfs.c we see this:

        TickType_t ticks_to_wait = portMAX_DELAY;
        if (timeout) {
            uint32_t timeout_ms = timeout->tv_sec * 1000 + timeout->tv_usec / 1000;
            ticks_to_wait = timeout_ms / portTICK_PERIOD_MS;
            ESP_LOGD(TAG, "timeout is %dms", timeout_ms);
        }
        ESP_LOGD(TAG, "waiting without calling socket_select");
        xSemaphoreTake(sel_sem.sem, ticks_to_wait);

Similar code can be found in components/lwip/port/esp32/freertos/sys_arch.c.

The code above clearly rounds down, while it should round up.

Sample code:

    struct timespec ts1;
    clock_gettime(CLOCK_MONOTONIC, &ts1);

    int efd = eventfd(0, 0);

    fd_set s1;
    FD_ZERO(&s1);
    FD_SET(efd, &s1);

    struct timeval tv;
    tv.tv_sec = 0;
    tv.tv_usec = 9999;
    select(1, &s1, NULL, NULL, &tv);

    struct timespec ts2;
    clock_gettime(CLOCK_MONOTONIC, &ts2);

    printf("ts1: %d %d, ts2: %d %d, diff: %d ns\n", (int)ts1.tv_sec, (int)ts1.tv_nsec, (int)ts2.tv_sec, (int)ts2.tv_nsec,
            (int)(ts2.tv_sec * 1000000000ULL + ts2.tv_nsec - (ts1.tv_sec * 1000000000ULL + ts1.tv_nsec)));

Output:

ts1: 2 99243000, ts2: 2 99478000, diff: 235000 ns

To be POSIX compliant, diff should have been at least 9999000 ns.

@espressif-bot espressif-bot added the Status: Opened Issue is new label Sep 3, 2021
@github-actions github-actions bot changed the title The select function times out too early The select function times out too early (IDFGH-5807) Sep 3, 2021
@o-marshmallow
Copy link
Collaborator

Hello @Emill ,
Thanks for your message, I am currently investigating and managing this issue.

@o-marshmallow o-marshmallow self-assigned this Oct 20, 2021
@espressif-bot espressif-bot added Status: In Progress Work is in progress and removed Status: Opened Issue is new labels Oct 21, 2021
@espressif-bot espressif-bot added Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: In Progress Work is in progress Resolution: NA Issue resolution is unavailable labels Nov 1, 2021
@o-marshmallow
Copy link
Collaborator

Hello @Emill ,
This bug has been fixed and merged internally, it will be synchronized to Github very soon.

@Emill
Copy link
Author

Emill commented Nov 3, 2021

Thanks for fixing this!

But as mentioned in the topic description, there is a similar place here: https://github.com/espressif/esp-idf/blob/master/components/lwip/port/esp32/freertos/sys_arch.c#L187. This function is called by lwip_select here: https://github.com/espressif/esp-lwip/blob/2c9c531f0a7e0ee536db9de4f9dc54e453712087/src/api/sockets.c#L2153, which is in turned called from esp_vfs_select here:

ret = socket_select(nfds, readfds, writefds, errorfds, timeout);

Could that result in the same issue as well?

@o-marshmallow
Copy link
Collaborator

@Emill Indeed, thanks for pointing this out. It is a different (sub)project, so it will be part of another commit.

@AxelLin
Copy link
Contributor

AxelLin commented Nov 9, 2021

@Emill Indeed, thanks for pointing this out. It is a different (sub)project, so it will be part of another commit.

@o-marshmallow
The fix for #7514 (comment) is in components/lwip/port/esp32/freertos/sys_arch.c which is part of esp-idf, which is not in sub project.

espressif-bot pushed a commit that referenced this issue Dec 9, 2021
`select` function will now round up the timeout passed as a parameter (if any).
It  makes it POSIX compliant.

* Closes #7514
espressif-bot pushed a commit that referenced this issue Dec 24, 2021
`select` function will now round up the timeout passed as a parameter (if any).
It  makes it POSIX compliant.

* Closes #7514
espressif-bot pushed a commit that referenced this issue Dec 26, 2021
`select` function will now round up the timeout passed as a parameter (if any).
It  makes it POSIX compliant.

* Closes #7514
espressif-bot pushed a commit that referenced this issue Dec 31, 2021
`select` function will now round up the timeout passed as a parameter (if any).
It  makes it POSIX compliant.

* Closes #7514
@AxelLin
Copy link
Contributor

AxelLin commented Jan 24, 2022

@Emill Indeed, thanks for pointing this out. It is a different (sub)project, so it will be part of another commit.

@o-marshmallow
Just remind that the fix for #7514 (comment) is not yet available.

espressif-bot pushed a commit that referenced this issue Feb 12, 2022
`lwip_select` uses `sys_arch_sem_wait` function making the assumption that it
is POSIX compliant. This commit makes that function wait for at least
timeout (milliseconds), as required by POSIX specification.

* Relates to #7514
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

No branches or pull requests

4 participants