On Tue, Jan 25, 2011 at 3:35 PM, Luke Kanies <[email protected]> wrote:
> Why even call this method if there's no @should value? > > The reason I removed the code that did exactly this was because I (thought > I) moved the check for that value into the resource_harness class, which > made the check in the insync? method redundant. > The issue is that the resource_harness class isn't the only call site. There are in fact about 10 call sites and about 25 implementations of the insync? method. This makes insync? a particularly risky method because if any one of the call sites makes an assumption that one of the implementations fails to satisfy (or vice versa), it will lead to a subtle bug that only occurs when that particular call site and that particular implementation happen to meet. That's precisely what happened in bug 5931: the property_fix method in the file provider was implicitly assuming that insync? would check whether @should was nil (and return true), but the group property's insync? method wasn't doing this check, and that caused an exception to be raised. It would be easy to fix just the failure documented in bug 5931 by either modifying property_fix so that it didn't call insync? when there was no @should value (as you suggest), or by modifying the group property's insync? method to check whether @should is nil. But Jesse and I were concerned that with so many call sites and so many implementations, there was a good chance that other bugs were lurking. A quick grep revealed that only 3 of the call sites checked whether @should was nil (and only 2 of them did so in an explicit way), so changing all the call sites to check @should didn't seem like the right solution; chances were good that someone would add another call site in the future and forget to do the check. Conversely, of the 25 or so implementations of insync?, about a quarter neglected to check whether @should was null (and many more did so only implicitly), so again, it seemed like if we fixed all of those implementations we would just risk someone making a mistake in the future. The standard way to solve a problem like this is to create a method that all calls pass through, so that you can put the check in one place and be guaranteed that it always gets run. That's what we did with safe_insync?. I think it's a much stronger solution because it dramatically decreases the likelihood that a bug like this will recur in the future. > > + # Otherwise delegate to the (possibly derived) insync? method. > > + insync?(is) > > + end > > + > > + def self.method_added(sym) > > + raise "Puppet::Property#safe_insync? shouldn't be overridden; please > override insync? instead" if sym == :safe_insync? > > + end > > This quite frightens me; have we done this anywhere else? Why the > significant concern on whether this particular method gets overridden? > We were concerned that a programmer unfamiliar with this particular bug might mistakenly override the safe_insync? method when we intended them to override insync?. It's of particular concern here in type-and-provider land because of the frequency with which users extend Puppet by writing their own types and providers. In languages like Java and C# it's easy to prevent these kind of errors by marking a method as "final". Since Ruby has no "final" keyword, we did some digging on the web to see if anyone had invented an analogous technique, and we found this trick with method_added. I don't think there's any reason we should be frightened about this. We should be reassured. There are plenty of places where our code correctness relies on some methods being overridden but not others. This is simply the first place where we've made the check explicit in the code so that if a programmer makes a mistake they will be alerted immediately rather than creating a subtle bug. -- 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.
