Atomic loading of Monticello packages

Stephane said that the biggest problem he had with Monticello was that
loading a package was not atomic.  It compiles methods one at a time,
and sometimes you need to load an entire package at once, because
loading half a package might break the compiler or some part of the
system that the package system depends on.

I decided to take a look at Monticello, because I've built other tools
that had to solve this problem.  I have attached a first attempt at a
solution.  This change makes loading a package atomic, but not
unloading it.  I will create a similar solution for unloading if
people like this.  I did not test it on any packages that require
atomic loading because I don't have any packages like that.  However,
it does run the Monticello tests, and I tested it on a couple of other
packages.  The tests only pass if you load the Make39Green.cs patch.
I don't know why, but I will figure it out some other time.

I am going to give a description of what I did.  Feel free to ignore
everything after this, but to just try out the code.  If you discover
a package that does not load properly,
please let me know.  I particularly want to know how this works on
packages that couldn't load before.

The Monticello code is fairly easy to read in spite of lacking
comments.  Class comments in particular would be useful.  There is a
large test suite that did a good job of pointing out my errors.

The key method is the basicLoad method in class MCPackageLoader.  The
original version is

basicLoad
        errorDefinitions := OrderedCollection new.
        [[additions do: [:ea | self tryToLoad: ea] displayingProgress: 'Loading...'.
        removals do: [:ea | ea unload] displayingProgress: 'Cleaning up...'.
        self shouldWarnAboutErrors ifTrue: [self warnAboutErrors].
        errorDefinitions do: [:ea | ea loadOver: (self obsoletionFor: ea)]
displayingProgress: 'Reloading...'.
        additions do: [:ea | ea postloadOver: (self obsoletionFor: ea)]
displayingProgress: 'Initializing...']
                on: InMidstOfFileinNotification
                do: [:n | n resume: true]]
                        ensure: [self flushChangesFile]

The problem with this is that classes and methods are loaded by the
tryToLoad: method, one at a time.  In fact, they are also added by the
loadOver: method a little later.  I think that an errorDefinition is a
method that started out in additions but couldn't compile, so the
PackageLoader will wait until all the classes are defined and then try
to compile it again.

tryToLoad: wraps loadOver: in an exception handler and, if anything
goes wrong, puts the offending method definition in errorDefinitions.
loadOver: calls a method in ClassDescription to compile the method,
and then calls the following method to insert the method in the class.

addAndClassifySelector: selector withMethod: compiledMethod
inProtocol: category notifying: requestor
        | priorMethodOrNil |
        priorMethodOrNil _ self compiledMethodAt: selector ifAbsent: [nil].
        self addSelectorSilently: selector withMethod: compiledMethod.
        SystemChangeNotifier uniqueInstance
                doSilently: [self organization classify: selector under: category].
        priorMethodOrNil isNil
                ifTrue: [SystemChangeNotifier uniqueInstance methodAdded:
compiledMethod selector: selector inProtocol: category class: self
requestor: requestor]
                ifFalse: [SystemChangeNotifier uniqueInstance methodChangedFrom:
priorMethodOrNil to: compiledMethod selector: selector inClass: self
requestor: requestor].

Note that addSelectorSilently:withMethod: finally inserts the method
into the class.  This is the step that must be avoided.  So, I changed
this method to leave out this message, and instead returned the
compiled method.  The new method is

MCaddAndClassifySelector: selector withMethod: compiledMethod
inProtocol: category notifying: requestor
        | priorMethodOrNil |
        priorMethodOrNil _ self compiledMethodAt: selector ifAbsent: [nil].
        SystemChangeNotifier uniqueInstance
                doSilently: [self organization classify: selector under: category].
        priorMethodOrNil isNil
                ifTrue: [SystemChangeNotifier uniqueInstance methodAdded:
compiledMethod selector: selector inProtocol: category class: self
requestor: requestor]
                ifFalse: [SystemChangeNotifier uniqueInstance methodChangedFrom:
priorMethodOrNil to: compiledMethod selector: selector inClass: self
requestor: requestor].

Note that the new method tells the SystemChangeNotifier that a method
was added but doesn't actually add it until later.  I hope this
doesn't cause a problem!  An alternative would be to first insert the
methods and then to notify the SystemChangeNotifier, but to do it all
at the end of basicLoad.

I changed the method that calls it to return the compiled method.

MCcompile: text classified: category withStamp: changeStamp notifying:
requestor logSource: logSource
  "Note that I return methodAndNode, while the old version returned a selector"
        | methodAndNode |
        methodAndNode := self compile: text asString classified: category
notifying: requestor
                                                        trailer: self defaultMethodTrailer ifFail: [^nil].
        logSource ifTrue: [
                self logMethodSource: text forMethodWithNode: methodAndNode
                        inCategory: category withStamp: changeStamp notifying: requestor.
        ].
        self MCaddAndClassifySelector: methodAndNode selector withMethod:
methodAndNode
                method inProtocol: category notifying: requestor.
        self instanceSide noteCompilationOf: methodAndNode selector meta:
self isClassSide.
        ^ methodAndNode

So now, we can define methods in MCMethodDefinition.  loadOver is a
method that compiles the method.  It was changed to return the method
rather than installing it.  load will both compile and install the
method.

loadOver: aDefinition
        "Note that this doesn't actually load the method, it just compiles it
and returns the compiled method for loading later."
        ^self actualClass
                MCcompile: source
                classified: category
                withStamp: timeStamp
                notifying: (SyntaxError new category: category)
                logSource: self actualClass acceptsLoggingOfCompilation


load
        | methodAndNode |
        methodAndNode := self loadOver: nil.
        self actualClass addSelectorSilently: selector withMethod: methodAndNode method

Here is the new version of basicLoad in MCPackageLoder.  Note that it
uses tryToLoad:in: to compile methods, but saves them in methodsToLoad
instead of installing them.  It collects some more methods from
errorDefinitions, and then installs all methods after it has processed
the errorDefinitions.  Note that the installing just uses pairsDo: on
OrderedCollection and addSelectorSilently:withMethod: on Class.  So,
it basically uses only Collection and Behavior methods.  I could make
it use even less code if that were important.

basicLoad
        | methodsToLoad |
        errorDefinitions := OrderedCollection new.
        methodsToLoad := OrderedCollection new: 10.
        [[additions do: [:ea | self tryToLoad: ea in: methodsToLoad]
displayingProgress: 'Loading...'.
        removals do: [:ea | ea unload] displayingProgress: 'Cleaning up...'.
        self shouldWarnAboutErrors ifTrue: [self warnAboutErrors].
        errorDefinitions do: [:ea | self load: ea in: methodsToLoad]
displayingProgress: 'Reloading...'.
        methodsToLoad pairsDo: [:class :eachMaN | class addSelectorSilently:
eachMaN selector withMethod: eachMaN method ].
        additions do: [:ea | ea postloadOver: (self obsoletionFor: ea)]
displayingProgress: 'Initializing...']
                on: InMidstOfFileinNotification
                do: [:n | n resume: true]]
                        ensure: [self flushChangesFile]

Did you had a look at the work of Colin?

   http://map.squeak.org/package/8830dcdc-9329-4190-80cc-dc614c55f3e0

In the description it says:

"SystemEditor is a mechanism for atomically loading a set of changes
to the system. It's intended to be used by tools such as that move
classes (or parts of classes) between images, such as Monticello,
Monticello2, change sets, fileIns etc.

It provides a familiar interface for making changes, but does not
execute the changes until it receives a #commit message. This makes it
easier to update existing tools to load their changes atomically."

--
Lukas Renggli
http://www.lukas-renggli.ch

On Nov 26, 2006, at 10:26 PM, Lukas Renggli wrote:

I posted about this in response to Ralph's last message, but the  
message seems to have dissipated in the ether somewhere. Here it is  
again:

On Nov 24, 2006, at 9:32 PM, Ralph Johnson wrote:


Yeah, sounds reasonable.

For MC2, I created a package called SystemEditor that uses roughly  
the same approach. It presents an "editing" interface with the same  
protocol as the real system. I'm sure it's not quite identical, but  
it's close enough that MC2 can have atomic loading turned on or off  
with a one-line change.

It probably wouldn't be much work to make MC1 load definitions  
through SystemEditor rather than SystemDictionary. More work would  
have to go into SystemEditor to make it solid enough for production  
use, but it's still probably less work than starting from scratch.

Colin

> "SystemEditor is a mechanism for atomically loading a set of changes
> to the system. It's intended to be used by tools such as that move
> classes (or parts of classes) between images, such as Monticello,
> Monticello2, change sets, fileIns etc.
>
> It provides a familiar interface for making changes, but does not
> execute the changes until it receives a #commit message. This makes it
> easier to update existing tools to load their changes atomically."

I am not sure how it is supposed to be used.  I couldn't find any
tests that actually tie it in to anything.

Are ClassEditor and MetaclassEditor supposed to replace Class and
Metaclass? It looks ot me like I would have to change the
MCPackageLoader to use ClassEditor and MetaclassEditor whenever it
currently uses Class and Metaclass.  Is that the idea?

-Ralph

On Nov 27, 2006, at 3:27 AM, Ralph Johnson wrote:



Yes, ClassEditor and MetaclassEditor are meant to be standins for  
real classes and metaclasses. However, the idea is to create a  
SystemEditor to use in place of a SystemDictionary. So you'd do  
something like "aSystemEditor at: #String" to fetch a ClassEditor on  
String.

On the other hand, SystemEditor is fairly ambitious in that it aims  
for truly atomic loading - all changes are installed using a single  
primitive call, so the only possibility for an incomplete load is  
misbehavior of the primitive. This code has lots of unit tests, but  
hasn't had much real world usage. I agree that your earlier approach  
of splitting loading into two passes gives a better balance of risk-
of-incomplete-loads vs risk-of-bugs-in-the-loading-code, at least for  
the short term. At some future point, we can look at either modifying  
MC1 to use SystemEditor, or moving to MC2.

I do intend to keep moving forward with both MC2 and SystemEditor,  
though progress is slow because I don't have much free time. It may  
or may not be useful in time to incorporate into the 3.10 process,  
but either way, I think your focus on process rather than technology  
is what's needed to keep Squeak vital.

Colin

I assume we'll move to MC2 eventually.  It is probably better to work
on that than to retrofit SystemEditor into MC1.  I'll just patch up
MC1 for the short term.

-Ralph

On Nov 27, 2006, at 6:20 PM, Ralph Johnson wrote:

> I assume we'll move to MC2 eventually.  It is probably better to work
> on that than to retrofit SystemEditor into MC1.  I'll just patch up
> MC1 for the short term.

Sounds good.

Colin