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

Deadlock on C function fsw_destroy_session if fsw_start_monitor was called. #137

Closed
Andrepuel opened this issue Sep 22, 2016 · 9 comments
Closed
Assignees
Labels
Milestone

Comments

@Andrepuel
Copy link

fsw_start_monitor will acquire the session mutex and will invoke session->monitor->start() with this mutex locked. The only way to stop the monitor (and thus release the mutex) using the C bindings is to destroy the session, but the fsw_destroy_session will also try to acquire the mutex.
An application that tries to stop the monitoring by destroying the session will deadlock.

@emcrisostomo
Copy link
Owner

What about line 865:

    session_lock.unlock();

where the mutex is released before invoking:

    session->monitor->start();

Please, feel free to reopen the issue if you have more information to add.

@Andrepuel
Copy link
Author

By session mutex, I meant https://github.com/emcrisostomo/fswatch/blob/master/libfswatch/src/libfswatch/c/libfswatch.cpp#L857.

unique_ptr<mutex>& sm = session_mutexes.at(handle);
lock_guard<mutex> lock_sm(*sm.get());

@emcrisostomo emcrisostomo reopened this Oct 19, 2016
@emcrisostomo
Copy link
Owner

I've patched the develop branch and now the session mutex is released before starting the monitor. Would you please test it?

@emcrisostomo emcrisostomo added this to the 1.10.0 milestone Oct 29, 2016
@emcrisostomo emcrisostomo self-assigned this Oct 29, 2016
@Andrepuel
Copy link
Author

Andrepuel commented Nov 6, 2016

In my scenario, I have two threads. The second thread will be running the fsw_start_monitor and the main thread will eventually invoke fsw_destroy_session.
I can confirm that the fsw_destroy_session is not deadlocking anymore on the main thread. However, the second thread will still be stuck in the fsw_start_monitor call due to the the issue #136.

@Andrepuel
Copy link
Author

By the way, if you don't mind me asking. Why did you choose the handle approach to identify the sessions in the C API?
With the alternative of returning an opaque struct pointer (struct FSW_SESSION; typedef struct FSW_SESSION* FSW_HANDLE;) you would be able to get rid of all the mutexes and the logic of maping a handle to the actual struct.

@emcrisostomo
Copy link
Owner

@Andrepuel:

I can confirm that the fsw_destroy_session is not deadlocking anymore on the main thread.

Thanks.

However, the second thread will still be stuck in the fsw_start_monitor call due to the the issue #136.

Please see #143. Basically, a monitor should be stopped explicitly by calling fsw_stop_monitor.

You can try out the release/1.10.0 branch.

@emcrisostomo
Copy link
Owner

@Andrepuel:

Why did you choose the handle approach to identify the sessions in the C API?

A choice that backfired. I already made that refactoring on release/1.10.0.

@emcrisostomo
Copy link
Owner

@Andrepuel,

This:

I can confirm that the fsw_destroy_session is not deadlocking anymore on the main thread.

and #143 should be enough to close this issue.

@Andrepuel
Copy link
Author

Using fsw_stop_monitor there is no deadlock anymore.

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

No branches or pull requests

2 participants