sc_sock_poll thread safety

[svladykin]

Fixes https://github.com/tezc/sc/issues/98

For all implementations:

• replaced err with a static thread-local sc_sock_poll_errstr to make sure sc_sock_poll_err() will return correct error message for different threads;
• allocate constant size result buffer for SC_SOCK_POLL_MAX_EVENTS events, which is 1024 by default.

For Linux/FreeBSD/MacOS (epoll and kqueue) :

• removed count and cap as well as events expanding to allow safe concurrent access (epoll and kqueue themselves are thread-safe);
• all the updates to sc_sock_fd are done before epoll_ctl/kevent call to avoid possible data races, if the kernel call failed rollback to the original state;
• made the code as similar as possible between the two implementations to simplify further development;

For Windows (WSAPoll):

• added lock to protect shared state;
• added a separate results buffer which is filled under the lock with sc_sock_poll_fill_results() to allow sc_sock_poll_event() and sc_sock_poll_data() access results outside of the lock;
• now the resulting number of events returned by sc_sock_poll_wait() is the actual number of events collected, not the capacity of events;
• added sc_sock_poll_data struct which contains index of fd in events, data pointer and also volatile edge_mask which is updated by atomic instructions to allow Edge-Triggered mode work without acquiring the lock;
• added data as a data structure that does not use realloc to expand: this is needed to have a stable pointer from sc_sock_fd to sc_sock_poll_data (again no need to acquire lock on every access);
• added ops buffer to store the socket operations submitted by other threads while polling is in progress;
• added polling flag that marks "polling is in progress" state and protects events from concurrent modifications;
• added wakeup_pipe which is registered to the same sc_sock_poll to be able to wake up the poller thread when submitting socket operations from parallel threads;
• if multiple operations are submitted by by parallel threads only the first one wakes up the poller thread, others do not need to do that;
• if multiple operations for the same socket were submitted before poller thread started processing them, they will be merged into a single operation;
• if a submitted socket operation aimed to completely delete the socket from sc_sock_poll then no reference to the socket fdt is held in ops, so it is safe to do sc_sock_term() and free() on the socket right after successful call to sc_sock_poll_del() in any thread without waiting until the poller thread actually processes that operation.
• fixed error handling for WSAPoll: use WSAGetLastError() instead of errno;
• fixed error handling for sc_sock_pipe_read() and sc_sock_pipe_write(): no need to check rc != len;
• fixed bug in sc_sock_pipe_init(): need to assign read fd to fdt;

[codecov[bot]]

Codecov Report

Merging #99 (7153a7f) into master (611a97d) will decrease coverage by 0.32%. The diff coverage is 89.18%.

Additional details and impacted files

[](https://codecov.io/gh/tezc/sc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan) ```diff @@ Coverage Diff @@ ## master #99 +/- ## ========================================== - Coverage 99.81% 99.49% -0.33% ========================================== Files 21 21 Lines 2198 2180 -18 Branches 388 385 -3 ========================================== - Hits 2194 2169 -25 - Misses 0 3 +3 - Partials 4 8 +4 ``` | [Impacted Files](https://codecov.io/gh/tezc/sc/pull/99?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan) | Coverage Δ | | |---|---|---| | [socket/sc\_sock.c](https://codecov.io/gh/tezc/sc/pull/99/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan#diff-c29ja2V0L3NjX3NvY2suYw==) | `98.58% <89.18%> (-1.42%)` | :arrow_down: | ------ [Continue to review full report at Codecov](https://codecov.io/gh/tezc/sc/pull/99?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan). > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan) > `Δ = absolute (impact)`, `ø = not affected`, `? = missing data` > Powered by [Codecov](https://codecov.io/gh/tezc/sc/pull/99?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan). Last update [611a97d...7153a7f](https://codecov.io/gh/tezc/sc/pull/99?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Ozan+Tezcan).

[svladykin]

@tezc Please review.

As expected epoll/kqueue implementations got simplified (no poll->events resizing), windows got complicated (but I tried to comment extensively).

There are no backward incompatible changes from the API point of view: poll->err was replaced with a static thread-local, so now it is thread safe without any change in semantics. Maybe it is worth using this approach for sockets as well to avoid having 128 bytes long err in every socket.

I understand the PR is large, I can try to split it into multiple smaller ones, but the biggest part there is Windows and it would be hard to split.

[tezc]

@svladykin Thanks, I'll have a look in a couple of days.

[svladykin]

@tezc Did you have a chance to take a look?

[tezc]

@svladykin Sorry for the delay. I was a bit busy these days. I had a quick look and pushed some minor changes mostly about style. I need some more time to wrap my head around, hopefully I'll complete the review by tomorrow.

[svladykin]

@tezc No rush, I just wanted to make sure you did not forget about this PR :)

[tezc]

@svladykin Sorry for the delay. I didn't have much time to review and probably it will take more time to wrap my head around the changes fully. Not to make this review longer, considering API is compatible and most changes are on Windows, I think we can merge it and I'll take a look in parallel and ask you questions if I need :)

• Thanks for the pipe & poll resizing fixes.
• Regarding using __thread for sock err, I didn't want to use anything compiler specific initially but socket code won't be quite portable anyway (at least not as much as a linkedlist impl). So, I guess we can use __thread for sockets as well.
• Overall, Windows side changes make sense but still I need more time to understand it fully. I can do that in parallel after we merge this anyway. I'll catch up in a few weeks hopefully.

Added a few comments in the PR regarding return value checks, after those, we can merge the PR I guess. Thanks for the PR and sorry for late review. Last month was a bit busy and looks like there is another busy month upcoming for me :)

[svladykin]

@tezc Thanks for the review. I will address your comments shortly.

[svladykin]

@tezc I have made the requested adjustments. Also I fixed a WSAPoll error handling issue on Windows. Please look.

[tezc]

@svladykin Thank you very much, looks good to me. We have %100 branch coverage on Linux but other OSes like Windows, damn there are many bugs. Thank you for the fixes.

Is there anything else we need to do for this PR? Btw could you add a few lines to the top comment and briefly describe the changes?

[svladykin]

@tezc I believe this PR is good to go. The epoll/kqueue changes seems to be safe, Windows is less safe, but if any issues arise I will fix them.