On Jan 26, 2011, at 11:50 AM, Paul Berry wrote: > 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's true that there are about 10 call sites, but the majority of those are either dead code (the 'Type#insync?' method, for instance), redundant code (the call in type/service.rb), or in the ResourceHarness class. > 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. I'd far prefer a solution that enhanced ResourceHarness with enough functionality that no one should ever need to call 'insync?' directly. E.g., you could add a method to either the harness or the Property class that used the Harness that did the appropriate dance. The reason I like this better is that you can actually provide control if everything goes through the harness, and refactoring becomes much easier, but you can't provide anything if everyone just directly calls the method. Given that my goal is to remove the 'retrieve' and 'sync' methods from the system, all of this area will end up getting a significant refactor anyway. > 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. Obviously this is a bit outside my normal area of advice, but my personal preference would be to rely on heavy documentation of the method rather than this kind of strictness. As it is, people can still override it by just overriding the method_added method, you've just required them to jump through stupid hoops to get what they want, which is exactly the kind of thing I continually run into in Rails. -- Levy's Law: The truth is always more interesting than your preconception of what it might be. --------------------------------------------------------------------- Luke Kanies -|- 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.
