On Jan 17, 2012, at 6:21 PM, Jeff McCune wrote: > On Fri, Jan 13, 2012 at 9:46 AM, Daniel Pittman <[email protected]> wrote: >> Option two, we patch this every place in the code that we fork, or >> direct them through `fork_closing_files` along the way. That will >> solve the problem everywhere that someone notices this is an issue, >> without question. > > I vote for this option. I think the solution is for those reviewing > code that goes in to be partially responsible for calling this out to > the authors. "Did you really mean to use fork instead of > fork_closing_files or popen?"
Another option is to use a local method that does the appropriate work and calls fork, and use that method instead of fork directly (i.e., only have that single call directly to 'fork'). Then any patch that includes 'fork' would be immediately suspect. >> The cost is that every single person who touches the code needs to >> know that `fork` behaves badly in some cases, and that they should >> call our wrapper instead. Which means that we are going to see this >> problem come back again and again. > > This is a high cost when reviewing code, but something to be mitigated > with a checklist. > > Also, I don't foresee us writing a lot of code that fork()'s which is > why I think the risk of monkey patching is greater than the risk of > forgetting to use fork_closing_files or popen. > > I can imagine myself cursing loudly when we run into an obscure bug > with another Gem that turns out to be caused by us overriding the > behavior of fork() in a way that doesn't show up in the stack trace. > "We did WHAT...!?" -- Luke Kanies | http://about.me/lak | http://puppetlabs.com/ | +1-615-594-8199 -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
