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

Thread start and stop revamp #2383

Closed
wants to merge 2 commits into from
Closed

Conversation

cypof
Copy link
Member

@cypof cypof commented Apr 28, 2015

Split off #2367. Now that threads can be waiting on locks or condition variables, it can be necessary to interrupt them to stop the app. This PR makes sure stopping a Caffe thread always calls interrupt before waiting on join. It also provides a method 'must_stop' that can be tested when running in a loop to exit on demand.

Start and stop methods now fail a CHECK instead of returning a boolean error, given that the application doesn't really have a way to recover from this type of error.

@flx42
Copy link
Contributor

flx42 commented Apr 29, 2015

Looks OK, but using CHECK instead of the boolean return value is debatable I think.


/** Will not return until the internal thread has exited. */
bool WaitForInternalThreadToExit();
Copy link

Choose a reason for hiding this comment

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

Should a public method be deleted without being labeled as @deprecated for some time?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's supposed to be implementation details of the thread mechanism used internally, so I would say yes.

cypof added 2 commits May 18, 2015 17:28
- Interrupt the thread before waiting on join
- Provide a method for looping threads to exit on demand
- CHECK if start and stop succeed instead of returning an error
@shelhamer
Copy link
Member

No longer needed with the switch in #4563.

@shelhamer shelhamer closed this Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants