[FIXED] EventLoop: Socket now closed only after event loop done polling #815
+192
−68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The socket was closed by the NATS library itself, which could cause some issue when the event loop thread would still be polling it. We now defer to the event loop adapter to make sure that the event loop library is done polling before invoking a new function that will take care of closing the socket. I have updated the event loop test (that simulates what our adapters are doing). The mockup event loop implementation is a bit too simplistic but should be ok for now. If we have issues, we would have to make the events a linked list. Resolves #814 Signed-off-by: Ivan Kozlovic <[email protected]>
|
@levb I would like if possible have @dmitry-sles-novikov build from this branch and test with it to see if that resolves the issue. @levb Assuming we get an ok, note that although this is a fix, we have to add a new API in nats.h and also the event loop adapters now will use this new API. So not sure if it will have to be a 3.10 and not a fix to 3.9. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #815 +/- ##
==========================================
+ Coverage 68.71% 70.53% +1.82%
==========================================
Files 39 47 +8
Lines 15207 15455 +248
Branches 3143 3183 +40
==========================================
+ Hits 10449 10901 +452
+ Misses 1700 1477 -223
- Partials 3058 3077 +19 ☔ View full report in Codecov by Sentry. |
levb
reviewed
Oct 24, 2024
| @@ -2146,9 +2146,21 @@ _evStopPolling(natsConnection *nc) | |||
|
|
|||
| nc->sockCtx.useEventLoop = false; | |||
| nc->el.writeAdded = false; | |||
| s = nc->opts->evCbs.read(nc->el.data, NATS_EVENT_ACTION_REMOVE); | |||
| // The "write" event is added and removed as we write, however, we always | |||
There was a problem hiding this comment.
👍 Right, matching the changes in natsLibevent_Read, thx for the explanation comment!
levb
approved these changes
Oct 24, 2024
|
@dmitry-sles-novikov standing by for your confirmation |
|
@levb Sorry, but I found a data race and am looking for its causes. |
|
@dmitry-sles-novikov What don't you post the report from the sanitizer so we can all have a look? |
|
@kozlovic I wanted to localize the problem, the reason could be in my code =). And I have localized problems now:
My guess: now we close file descriptor after calling of Although the documentation says that this is acceptable:
My guess: when we schedule detach event to event loop, we can call free on scheduler before completion of int uv_async_send(uv_async_t* handle) {
_Atomic int* pending;
_Atomic int* busy;
pending = (_Atomic int*) &handle->pending;
busy = (_Atomic int*) &handle->u.fd;
/* Do a cheap read first. */
if (atomic_load_explicit(pending, memory_order_relaxed) != 0)
return 0;
/* Set the loop to busy. */
atomic_fetch_add(busy, 1);
/* Wake up the other thread's event loop. */
if (atomic_exchange(pending, 1) == 0)
uv__async_send(handle->loop);
-->//calling free(scheduler) here in event loop thread
/* Set the loop to not-busy. */
--->atomic_fetch_add(busy, -1); /// using member u.fd from scheduler
return 0;
} |
|
@dmitry-sles-novikov So I have spent some time reviewing this and also compiled the libuv library in Debug mode and added some print statements to show you that the case 2 is bogus. I will explain later. But first note (that may apply to both cases), it looks like the For case 1, unless you have truncated the stack for the "Previous write", I don't see any code from the NATS C library, so I would say that something else is racing with the libuv adapter, but not NATS C Client, which by the way owns the fd, so you would have to look at what the "DB::..." code is doing. I can't help there. For case 2, I have searched and I believe that I am doing the right thing to properly dispose of the I set a break point in The You can look at uv__async_spin and see that it waits for that busy flag (an atomic int) to be down to 0 before proceeding. You can change to So again, given that the NATS C library line numbers do not match this PR, it could be that you do not have the up-to-date libuv.h and NATS C client library (or your app is not linked to those), or the sanitizer is reporting a false positive (or real but because your platform is not implementing correctly the atomics?). Regardless, based on the libuv code reading, the NATS C client is doing the right thing and there is nothing to fix (in addition to what is done in this PR). |
|
@kozlovic Yes, I may be wrong in my assumptions. |
|
@levb LGTM |
|
Thank you very much for your help =) |
|
@dmitry-sles-novikov PS: I realize that your "case 2" stack looks a bit suspicious, in that it looks like it was tearing down. The line numbers do not correspond to my version which is latest on |
|
I had a quick look at the use of NATS and libuv in ClickHouse (and your PR) and this is too complex for me to comment, but I wonder if this is used properly. Some notes (and they may not be relevant, but again, too complex for me): You can't call |
|
@kozlovic clickhouse uses its own fork for nats.c, so i had to rebase your changes to it (branch fix_814 in repo). And for libuv used own fork too. That's why it took me so long to check, because I couldn't be sure where exactly the problems were. P.S. For case 1: in clickhouse the socket is always closed from another thread, so uvAsyncDetach is called asynchronously by publishing an event to the event loop thread. Maybe this is important |
Again, the socket should be owned by the NATS library, and it is closed in some cases due to stale connection or when an error occurs and will start a thread to reconnect, or when calling natsConnection_Close(). In any case, as long as the forks do similar things that the main repo, the socket close should now always happen from the event loop thread. |
I fixed it in my PR, previous version of code call natsLibuv_SetThreadLocalLoop from all threads
At the moment the sanitizer is triggered the event loop is still running. I am checked this |
|
I'll try to run tests on your libuv version, but it will take some time |
|
@kozlovic Full sanitizer output with libuv rebased to clickhouse libuv fork Data race in epoll_ctl(loop->backend_fd, EPOLL_CTL_DEL, fd, &dummy);Data race in free(nle->scheduler);Data race in free(nle); |
|
@dmitry-sles-novikov For the first data race, again, this is not a race with NATS library and libuv, not sure how ClickHouse has access to the file descriptor owned by the NATS C client. For the other two, I will investigate further. I think it has to do with the fact that you run the loop with no wait. I need to run some experiments and will let you know. For the first, please check what ClickHouse is doing with the NATS C Client file descriptor. |
The file descriptor does not belong to the NATS C client, we closed it here and allowed it to be reused:: Location is file descriptor 82 destroyed by thread T546 at: From documentation:
But at this point we still have an non closed poll handle associated with this fd. We will close the poll handle later in the attach or detach function: static void closeSchedulerCb(uv_handle_t* scheduler)
{
natsLibuvEvents *nle = (natsLibuvEvents*) scheduler->data;
uv_close((uv_handle_t*) nle->handle, finalCloseCb);
}
static natsStatus
uvAsyncAttach(natsLibuvEvents *nle)
{
natsStatus s = NATS_OK;
// We are reconnecting, destroy the old handle, create a new one
if (nle->handle != NULL)
{
uv_close((uv_handle_t*) nle->handle, uvHandleClosedCb);
nle->handle = NULL;
}
.....
}
void uv_close(uv_handle_t* handle, uv_close_cb close_cb) {
assert(!uv__is_closing(handle));
handle->flags |= UV_HANDLE_CLOSING;
handle->close_cb = close_cb;
switch (handle->type) {
...
case UV_POLL:
uv__poll_close((uv_poll_t*)handle);
break;
...
}
uv__make_close_pending(handle);
}
void uv__poll_close(uv_poll_t* handle) {
uv__poll_stop(handle);
}
static void uv__poll_stop(uv_poll_t* handle) {
uv__io_stop(handle->loop,
&handle->io_watcher,
POLLIN | POLLOUT | UV__POLLRDHUP | UV__POLLPRI);
uv__handle_stop(handle);
uv__platform_invalidate_fd(handle->loop, handle->io_watcher.fd);
}
void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) {
uv__loop_internal_fields_t* lfields;
struct uv__invalidate* inv;
struct epoll_event dummy;
int i;
lfields = uv__get_internal_fields(loop);
inv = lfields->inv;
/* Invalidate events with same file descriptor */
if (inv != NULL)
for (i = 0; i < inv->nfds; i++)
if (inv->events[i].data.fd == fd)
inv->events[i].data.fd = -1;
/* Remove the file descriptor from the epoll.
* This avoids a problem where the same file description remains open
* in another process, causing repeated junk epoll events.
*
* We pass in a dummy epoll_event, to work around a bug in old kernels.
*
* Work around a bug in kernels 3.10 to 3.19 where passing a struct that
* has the EPOLLWAKEUP flag set generates spurious audit syslog warnings.
*/
memset(&dummy, 0, sizeof(dummy));
if (inv == NULL) {
epoll_ctl(loop->backend_fd, EPOLL_CTL_DEL, fd, &dummy);
} else {
uv__epoll_ctl_prep(loop->backend_fd,
&lfields->ctl,
inv->prep,
EPOLL_CTL_DEL,
fd,
&dummy);
}
}
Sanitizer detects how we pass an already closed and reused file descriptor into a system call along with another thread that is now the owner of the file descriptor 82 P.S. Maybe I failed to explain again and I give up =)))) |
But the point of this PR was that I was doing a
I understand your explanation, but I can't understand how this happen with the changes in this PR. Please don't give up on me and try to explain one more time ;-) For the cases 2 and 3, I have an idea of what it could be, so will work on that. |
|
I guess an approach would be to do the |
You are right!!! I didn't pay attention.
It looks like that |
- Move the `uv_async_send` under our lock to avoid crash/race - Replace `uv_poll_stop` with `uv_close` and deal with nle->handle in place and not again in the final close callback. Signed-off-by: Ivan Kozlovic <[email protected]>
|
@dmitry-sles-novikov I just pushed a new commit that hopefully addresses the issue you have observed. I believe it will for sure for cases 2 and 3, and I hope solves also case 1. That one, I am not sure what I was doing before was wrong (since you see that stop is invalidating already), but at least it seem now more correct: when the connection is closed (or reconnecting) we are closing this handle (and closing this socket). Prior, we would have the stop (and close of socket) and then uv_close on final close. If you could rebuild with this new commit and let me know how it looks it would be much appreciated. Thank you for your efforts and patience! |
|
@kozlovic All tests passed, everything is great! Thank you very much for your help and patience, I hope my help was useful. It was a pleasure to work with you |
This is great news!
Pleasure is mutual. And thank you for your persistence since that made me have a closer look at the way the final close was happening, which was wrong but I did not see that earlier. |
|
@levb PR is complete and @dmitry-sles-novikov confirmed that there is no longer any sanitizer issues. You can merge whenever you have the time. |
github-actions bot
pushed a commit
that referenced
this pull request
Nov 5, 2024
…only after event loop done polling (#815)
|
Hi, @levb! Could you please share plans on releasing this fix? |
|
@zenoway I am on a family vacation ATM, with limited access. I will cut the release as soon as I am back in December. |
|
@levb OK, thank you for the update! PS. Have a great time on vacation! :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The socket was closed by the NATS library itself, which could cause some issue when the event loop thread would still be polling it. We now defer to the event loop adapter to make sure that the event loop library is done polling before invoking a new function that will take care of closing the socket.
I have updated the event loop test (that simulates what our adapters are doing). The mockup event loop implementation is a bit too simplistic but should be ok for now. If we have issues, we would have to make the events a linked list.
Resolves #814
Signed-off-by: Ivan Kozlovic [email protected]