fsevents: fix clever rescheduling #946

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@indutny
Contributor
indutny commented Oct 2, 2013

There're could be a situation, where one fsevents handle gets created
and another one is destroyed simultaneously. In such cases
fsevent_need_reschedule will be set to 1 twice and reset only once,
leaving handle destructor hanging in uv_sem_wait().

se nodejs/node-v0.x-archive#6296

/cc @bnoordhuis

@indutny
Contributor
indutny commented Oct 2, 2013

Added test.

@bnoordhuis bnoordhuis commented on an outdated diff Oct 2, 2013
test/test-fs-event.c
@@ -502,3 +502,31 @@ static void fs_event_cb_close(uv_fs_event_t* handle, const char* filename,
}
#endif /* HAVE_KQUEUE */
+
+TEST_IMPL(fs_event_start_and_close) {
+ uv_loop_t* loop;
+ static uv_fs_event_t fs_event1;
+ static uv_fs_event_t fs_event2;
@bnoordhuis
bnoordhuis Oct 2, 2013 Contributor

Why static?

@bnoordhuis bnoordhuis and 1 other commented on an outdated diff Oct 2, 2013
src/unix/fsevents.c
return;
- state->fsevent_need_reschedule = 0;
+ }
+ state->fsevent_need_reschedule--;
+ uv_mutex_unlock(&state->fsevent_mutex);
@bnoordhuis
bnoordhuis Oct 2, 2013 Contributor

Are similar issues possible with e.g. the call to uv__fsevents_create_stream() a few lines down? It's also not guarded by the mutex.

@indutny
indutny Oct 2, 2013 Contributor

Not really, but I just realized that this fix breaks this optimization.

@bnoordhuis
bnoordhuis Oct 2, 2013 Contributor

Another thing, if all accesses to state->fsevent_need_reschedule are now guarded by the mutex, then it doesn't need to be volatile anymore.

@indutny
indutny Oct 2, 2013 Contributor

Ok, figured it out. Going to push update in a bit.

@indutny indutny fsevents: fix clever rescheduling
There're could be a situation, where one fsevents handle gets created
and another one is destroyed simultaneously. In such cases
`fsevent_need_reschedule` will be set to 1 twice and reset only once,
leaving handle destructor hanging in uv_sem_wait().
97af9ec
@indutny
Contributor
indutny commented Oct 2, 2013

Fixed all nits, and improved patch to keep clever hack working.

@bnoordhuis bnoordhuis commented on the diff Oct 2, 2013
src/unix/fsevents.c
@@ -317,9 +317,13 @@ static void uv__fsevents_reschedule(uv_fs_event_t* handle) {
/* Optimization to prevent O(n^2) time spent when starting to watch
* many files simultaneously
*/
- if (!state->fsevent_need_reschedule)
- return;
+ uv_mutex_lock(&state->fsevent_mutex);
+ if (state->fsevent_need_reschedule == 0) {
@bnoordhuis
bnoordhuis Oct 2, 2013 Contributor

My comment about fsevent_need_reschedule still stands. It doesn't need to be volatile anymore, right?

@bnoordhuis
Contributor

One comment but otherwise LGTM.

@indutny indutny closed this Oct 2, 2013
@indutny indutny deleted the unknown repository branch Oct 2, 2013
@indutny
Contributor
indutny commented Oct 5, 2013

Landed in master in 429bb80.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment