[8.0.2] Problems loading Seaside twice

[jtuchel]

This issue has been around in 8.0.1 already and I was hoping it's going to go away in 8.0.2. But it's still there.

If you try to load the Map z.ST:Server Smalltalk (SST) - Seaside twice, you'll get a WAAttributeNotFound exception.

This is what happens:
in SeasideDevelopmentCore class>>#loaded all classes in the application that implement the class method #initialize are sent #initialize. Then WAWalkbackErrorHandler class>>#initialize tries to register itself as the default Seaside ExceptionHandler class:

Code: Select all

initialize
   WAAdmin applicationExceptionHandlingDefaults at: #exceptionHandler put: self

This leads to a problem because of the following (maybe strange) implementation of at:put: (look at the comment!)

Code: Select all

at: aSymbol put: anObject
   "It would be faster to simply add the value to the dictionary and
   implement #takeValue:forAttribute: in terms of this method, but
   config options are set only rarely and this ensures you are setting
   a value for an attribute that actually exists."

   ^ self takeValue: anObject forAttribute: (self attributeAt: aSymbol)

This leads to the abovementioned exception to be thrown, because the attribute #exceptionHandler has been cleaned earlier:

Code: Select all

attributeAt: aSymbol
   ^ self attributeAt: aSymbol ifAbsent: [ WAAttributeNotFound signalWithKey: aSymbol ]

.

So what could be done to solve this?
a) The attribute needs to be created before WAWalkbackErrorHandler class>>#initialize is called. This could be done in the #loaded method and could therefor be Seaside-independent (since #loaded is VAST specific afaik). But it would be a bit strange to do so
b) at:put: is modified to use #storeExpression:forAttribute: directly instead of #takeValue:forAttribute:. But this would definitely be a change to Seaside code and would (I guess) make porting harder, unless the Seaside team agrees to adapt this
c) The attribute itself is made to lazily initialize itself to WAWalkbackErrorHandler - this again would need a change in Seaside's code base
d) something clever that I cannot think of right now.

Why am I complaining? Because this error makes loading code in a tree of required maps (one-click-loading) harder, since Seaside itself may not be part of the required maps structure, because it cannot be loaded twice. I don't really like that

Regards

Joachim

[jtuchel]

...this brings up painful memories... and realities well

This issue also results in the fact that you cannot load "z.St: Server Smalltalk (SST) Seaside Testing" On top of the Seaside map, because the Seaside Map is a required map of the Testing Map.

Workaround: if you want the Seaside Tests loaded intio an image, start with a fresh image and load the Seaside Testing map with Required Maps. Thus teh Seaside Map only gets loaded once.

I remember this topic has been discussed here on the forum already, and obviously, the problem was not solved in 8.0.2..

Regards

Joachim

[benvandijk]

Hi Joachim,

First of all i totally agree with you: It should be possible to load any configmap twice.
I am surprised this bug is not fixed in 8.0.2.

Secondly, In my opinion the loaded methods should not perform the initialization of seaside classes.
This should be done when a seaside server is started first time i.e. only when its needed.

We do it anyway when we start our seaside application.

Lastly, i only use the z- configmaos to try out new features in a clean image.
Our structure configmaps contain Seaside Core, Seaside Test configmaps which luckily have no prereqs.

Greetings, Ben.

P.S. We had a great time in Stuttgart, many thanks for that.

[jtuchel]

Ben,

Secondly, In my opinion the loaded methods should not perform the initialization of seaside classes.
This should be done when a seaside server is started first time i.e. only when its needed.

I must admit I never thought about this. I just assumed this is code "inherited" from Seaside on Pharo. But you are perfectly right. There's no reason why this needs to be done at load time, especially if there is some significant likeliness that real applications will override this setting anyways...

Lastly, i only use the z- configmaos to try out new features in a clean image.
Our structure configmaps contain Seaside Core, Seaside Test configmaps which luckily have no prereqs.

In contrast, I like to use the z.ST... maps, since they reduce the number of dependencies of my application code from VAST code. There's always a chance you mix up some VAST 8.0.1 maps with 8.0.2 maps once you upgrade to a new VAST version. Maybe this is not such a strong argument...

Joachim

[jtuchel]

Hi VAST team,

I think Ben has brought up a great point here: these initializations are not related to loading seaside and therefor should not be part of it. The more important point is that it makes configuration management really hard because you need to work around re-loading the Seaside base code for no apparent reason.

I would hope for an AR number and (more importantly) a fix in VAST 8.03. Any thoughts?

Joachim

First of all i totally agree with you: It should be possible to load any configmap twice.
I am surprised this bug is not fixed in 8.0.2.

Secondly, In my opinion the loaded methods should not perform the initialization of seaside classes.
This should be done when a seaside server is started first time i.e. only when its needed.

Greetings, Ben.

[jtuchel]

Hi,

the problem is still present in 8.0.3. You cannot load SeasideTesting (or reload Seaside) if Seaside is already loaded due to the fact that WAWalkbackErrorHandler class>>initialize calls #at:put: in WASharedConfiguration which expects the setting #exceptionHandler to be present.

So please be aware that if you want Seaside Testing in an image, do not load Seaside first.

Joachim

[TriSebastian]

Hi Joachim,

we already talked about it. In Squeak and Pharo everything get's persisted in the image. Have a look at all senders of copy within the Seaside applcations.
You will hardly get rid of certain instances once you want to.

Please try this. I didn't actually test it properly, but it might be enough to simply reload Seaside into the image.

WAWalkbackErrorHandler class>>#initialize
WAAdmin configuredExceptionHandlingDefaults: false.
WAAdmin applicationExceptionHandlingDefaults at: #exceptionHandler put: self

You'll need to implement this configuredExceptionHandlingDefaults: setter for the class instance variable at WAAdmin...

Don't forget to restart your ServerAdaptor

Cheers!
Sebastian

[TriSebastian]

Me again

The biggest issue here is the GRPlatform and it's "defined" behavior. GRVASTPlatform registers it's self into the startup routine and here we have no real reinitialization of Seaside application state due to all the "copy" calls.

Solving this might give Seaside under VAST a much more mature handling. Not only during developement, but also during packaging.

Sebastian

[jtuchel]

Sebastian,

we already talked about it. In Squeak and Pharo everything get's persisted in the image. Have a look at all senders of copy within the Seaside applcations. You will hardly get rid of certain instances once you want to.

I somehow don't understand what exactly you want to tell me with that...

Please try this. I didn't actually test it properly, but it might be enough to simply reload Seaside into the image.

WAWalkbackErrorHandler class>>#initialize
WAAdmin configuredExceptionHandlingDefaults: false.
WAAdmin applicationExceptionHandlingDefaults at: #exceptionHandler put: self

I still don't get it. Let's assume WAWalkbackErrorHandler class>>#initialize is inherited/ported from Pharo without any modifications. And also let's assume that the same is true for WAApplicationDefaults>>#at:put:.
This leads me to the conclusion that:
1. The same error would occur in Pharo if WAWalkbackErrorHandler class>>#initialize gets executed before someone added the #exceptionHandler preference
2. In Pharo, class side #initialize's are not executed at package load time

Adding code like you suggest it should then be pushed back into the Seaside core code to make porting easier, or we need a way of avoiding WAWalkbackErrorHandler class>>#initialize on load (which is probably not really easy).

But in the end I guess the strange way of throwing an exception when a previously absent preference is set should be removed from Seaside.

Joachim