Skip to content
This repository has been archived by the owner on Mar 7, 2023. It is now read-only.

Fixed a crash after stopMonitoring on Linux in Electron #138

Closed
wants to merge 1 commit into from
Closed

Fixed a crash after stopMonitoring on Linux in Electron #138

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions src/detection_linux.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,13 @@ void Stop() {
return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone wants to submit a new PR with the review addressed and tested, we can move this forward more. Credit still goes to all involved (@antelle 🙇).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just adding a reference here, too. I think I've made the requested changes: #162

}

isRunning = false;

uv_mutex_destroy(&notify_mutex);
uv_signal_stop(&int_signal);
uv_signal_stop(&term_signal);
uv_close((uv_handle_t *) &async_handler, NULL);
uv_cond_destroy(&notifyDeviceHandled);

udev_monitor_unref(mon);
udev_unref(udev);

isRunning = false;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an exact behavior we're going for by moving this below (please explain and document)? Or is just stylistic preference?

I could see it maybe mattering if the uv_xxx stuff takes time to run and Stop is called multiple times, in the previous code, we wouldn't do all of the cleanup multiple times.

Copy link
Contributor

@Julusian Julusian Jan 20, 2022 •
edited by MadLittleMods
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I agree that changing isRunning at the top makes more sense, as we want the loop to exit asap, and not start another iteration while we start cleaning up

}

void InitDetection() {
Expand Down Expand Up @@ -244,6 +241,9 @@ static void cbWork(uv_work_t *req) {
}

static void cbAfter(uv_work_t *req, int status) {
udev_monitor_unref(mon);
udev_unref(udev);
Copy link
Owner

@MadLittleMods MadLittleMods Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just move these above uv_mutex_destroy in Stop? It seems like it would have the same effect.

I assume there is nuance between cbAfter vs cbTerminate that we want this difference. Why don't we want to call udev_unref(udev); in cbTerminate?

Copy link
Contributor

@Julusian Julusian Jan 20, 2022 •
edited by MadLittleMods
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from some googling, I think the flow is that after cbWork(running on the threadpool) has finished, cbAfter(run on the event loop) will be called.
cbTerminate will be called (I assume on the event loop) when the program wishes to terminate.

It sounds to me like cbTerminate wants to simply set isRunning to false, and let cbAfter do all the cleanup.
I think the problem is that the poll loop is in the middle of a poll at the point where udev_monitor_unref(mon); is called, causing undefined behaviour

A follow up question is why does cbAfter/cbTerminate do the cleanup instead of doing it at the end of cbWork after the loop has stopped?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds to me like cbTerminate wants to simply set isRunning to false, and let cbAfter do all the cleanup.
I think the problem is that the poll loop is in the middle of a poll at the point where udev_monitor_unref(mon); is called, causing undefined behaviour

@Julusian This sounds reasonable 👍

A follow up question is why does cbAfter/cbTerminate do the cleanup instead of doing it at the end of cbWork after the loop has stopped?

This also seems reasonable as long as those clean-up functions can be called there 🤷

Appreciate the context and confidence here ❤️


Stop();
}

Expand Down