Skip to content

Conversation

@GitMoDu
Copy link

@GitMoDu GitMoDu commented Jan 24, 2018

Discussion here: #14

@wizard97
Copy link
Owner

wizard97 commented Jan 25, 2018

Appreciate you following through and making an example.

Looks mostly good, a few suggestions:

  • Can you instead of naming your class BlinkerProcesses, name it something like BlinkerManager? I saw the word process in the name and assumed it inherited from the base process which confused me for a bit.

  • Can you add a deconstructor to the BlinkerManager that deletes all the BlinkProcesses from the scheduler? If you don't do this and the BlinkerManager goes out of scope, the Scheduler will contain references to the now nonexistent BlinkProcesses, which would add all sorts of weird behavior and/or crashing.

@GitMoDu
Copy link
Author

GitMoDu commented Jan 25, 2018

  1. Sure, it is confusing. I think I started by extending the Process class but then realized it might be more useful to have an example without direct inheritance.

  2. Didn't catch that part, I'll add it.

@wizard97
Copy link
Owner

I think it would actually be pretty cool if you could think of an example where this "managing process" is actually a process itself. Maybe you make the manager process change the way the LEDS blink every 10 seconds or something? That would be pretty cool.

@GitMoDu
Copy link
Author

GitMoDu commented Jan 26, 2018

Can you add a deconstructor to the BlinkerManager that deletes all the BlinkProcesses from the scheduler?

Do you mean just overriding the cleanUp() method, or catching the ~Process() to remove the sub-processes from the scheduler?

@wizard97
Copy link
Owner

wizard97 commented Jan 26, 2018

Definitely needs to be caught in cleanUp().

I actually realized calling cleanUp() on the child procs in the deconstructor isn't safe. As calling cleanUp() only queues the request to destroy a process. By the time the request actually gets processed the childprocs themselves will be out of scope since the deconstructor already would have run.

@GitMoDu
Copy link
Author

GitMoDu commented Feb 27, 2018

Updated with clean-up.

@wizard97
Copy link
Owner

Sorry, I completely missed your updates!

Everything looks good, besides the cleanup of the child processes. I can't think of a way to do it that is clean and also safe.

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.

2 participants