-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WAFlowFunctionalTest fails on GemStone #1198
Comments
@jbrichau suggested:
Yeeeaahhh! That fixes it! |
@dassi Glad that works for you.
@dassi I noticed this as well, but when the continuation mechanism fails, I'm not sure there is much to do... |
From this stack:
it seems that this issue is coming up because the environment dictionary (where the dynamic variable is stored in GsProcess) is not being handled correctly in the presence of continuations ... I've talked to @AllenOtis and internally we do not preserve the environment dictionary when we restore a full continuation, but we DO preserve the environment dictionary when we restore a partial continuation. To be clear, the environment dictionary that was associated with the original process that the partial continuation was associated with is NOT preserved. We DO preserve the environment dictionary of the process into which the partiali continuation is restored ... Looking at this stack I see that Dynamic Variables are defined (GrDynamicVairable class>>use:during:) in the frames: 35*, 62*, 75, 84*, 94*, 104* ... the *'d frames are where WACurrentRequestContext is being called ... all but one (75) have the same key so the WACurrentRequestContext from frame 35 should win ... The WARequestContextNotFound is signaled because no entry exists in the environment dictionary of the current GsProcess. Thinking that we should be able to reproduce this bug in a simple test case based on the WAPartialContinuationTest, I've fiddled around a bit and finally think that I may have reproduced the bug with a variant of WAPartialContinuationTest>>testReentrant: testReentrant
| kk x |
self
assert:
(self
mark: [
2
*
(self
callcc: [ :cc |
kk := cc.
2 ]) ])
= 4.
WADynamicVariable
use: 6
during: [
self assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12.
self
assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12.
self
assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12 ] When I run this test I get a failure in the second invocation of the continuation and the value returned by WADynamicVariable is Anyway we won't be able to do more work on this bug until next week. |
Much appreciated @dalehenrich! If I understand correct, the If I change your test (just for my understanding purposes), to show that the process environment is thrown away. From within the test, I can restore the environment to make the test work but I could not do that in the implementation of | kk x |
self
assert:
(self
mark: [
2
*
(self
callcc: [ :cc |
kk := cc.
2 ]) ])
= 4.
WADynamicVariable
use: 6
during: [
| env p |
env := Processor activeProcess environment.
p := Processor activeProcess.
self
assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12.
self deny: Processor activeProcess == p.
Processor activeProcess environment: env.
self
assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12.
Processor activeProcess environment: env.
self
assert: (self mark: [ kk value: (x := WADynamicVariable value) ]) = 12 ] |
@jbrichau, it looks like it might be possible to arrange to preserve the environment dictionary across the GsProcess>>installPartialContinuation:... by monkeying with WAPartialContinuation>>value:. There's a setter for the environment dictionary in GsProcess, so that might be a straightforward patch ... I don't have the cycles today to check that out, but it might be a better patch than reverting the implementation of WADynamicVariable. |
@dalehenrich I tried that but my monkeying was not working. but will give it a second try once I get back to my console today ;) |
I tried this by setting the environment of the ...
partial environment: Processor activeProcess environment.
^ GsProcess
installPartialContinuation: partial
atLevel: frameIndex
value: anObject |
Some more monkeying today brought me to the following hack in Seaside to make the combination of DynamicVariable and partial continuations work in Gemstone. This fixes the WAFlowFunctionalTest example but might not be complete (I'll get back to it later). The following two methods need changing: WAComponent>>wait: aBlock
"Evaluate aBlock and pass in a continuation that can be evaluated to answer to the place where this very method was called."
| dict |
dict := GRPlatform current seasideSuspendFlowDo: [ :cc | aBlock value: cc ].
Processor activeProcess environment: (dict at: 'env').
^ dict at: 'value' WAPartialContinuation>>value: anObject
| marker frameIndex |
marker := self markerBlock value.
marker isNil
ifTrue: [
marker := (GsProcess _frameContentsAt: 2) at: 1.
frameIndex := 2 ]
ifFalse: [ frameIndex := self class findFrameIndexFor: marker ].
^ GsProcess
installPartialContinuation: partial
atLevel: frameIndex
value:
(Dictionary new
at: 'env' put: Processor activeProcess environment;
at: 'value' put: anObject;
yourself) The essence is to 'pass through' the environment of the process in which the partial continuation is restored such that its environment is still the same after the partial continuation is restored. @dalehenrich I need to verify if this is a complete and sound workaround. The other question would be: would it be feasible to have the environment copying made a possibility of the |
An implementation of the same workaround that fits in Gemstone-only packages is made in #1201 I also have the Parasol automation for these tests ready in #1199 but some other failures appear in the Gemstone tests because of that. It's probably related to the presence of Parasol so I will check later this week. @dalehenrich in case you are able to cast an eye. Would be great! |
@jbrichau if I'm reading the I'm busy today (yesterday was a holiday), but I will take #1199 for a spin tomorrow ... |
Hey @dalehenrich, as I am trying to finalize PR #1201 by fixing the failing tests, I am left wondering what See https://travis-ci.org/github/SeasideSt/Seaside/builds/692678006 for the last build. The failing tests are 'normal'. The context where the partial continuation resumes does not deal with the monkey patch of passing the environment in a dictionary. I'm still fiddling. |
Spelunking in the git history:
If I recall that far back (sketchy at best) I copied the pharo continuation tests for GemStone and got them to work ... and then relied on keeping the tests passin from that point on ... If this wasn't a pharo test, then I must have created it and misspelled Alternate. I don't know when we started supporting partial continuations (and I'm not going to try to spelunk through Monticello history), ut I wouldn't be surprised if was experimenting with different stack markers in an attempt to reproduce bugs we were seeing in running Seaside and Seaside tests ... if I had passing tests I would have left them in even if they didn't reproduce the error ... If it doesn't represent a current use case, then I'm not sure it is worth keeping ... |
I finalized the implementation of a workaround for this issue in #1201 @dassi Do you see an opportunity to test the fixes done in that branch on your application before I add them to the master? Thanks again for reporting and sorry for the hassle. It took me a little longer to fix the issue because I also have taken the time to add the functional test automation to the testsuite. This will at least help to prevent this kind of mistakes in the future. It will also help if I actually use the latest version of Seaside in production in the future as well ;-) @dalehenrich Any chance of being able to pass the environment to the primitive in future GemStone versions? Or if you see a better workaround, I am all ears. BTW, notice that builds for GemStone 3.5.0 have started failing on loading FileTree... (see https://travis-ci.org/github/SeasideSt/Seaside/jobs/702633712 ) This is strange because I do not see that anywhere else where I am loading in GemStone 3.5.0. And it should not have anything to do with changes for this issue. I have a working load with the parasol tests added in this build of a month ago: https://travis-ci.org/github/SeasideSt/Seaside/jobs/690603830 |
I'm going to be push my Rowan work on the stack for awhile while I spend some quality time with GLASS, we've got the SeasideSt/Grease#105, Metacello/metacello#514, and https://travis-ci.org/github/GsDevKit/zinc/builds/702795787 issues outstanding ... Interestingly enough I just did a GsDevKit_home build with the fresh clones of all of the projects including FileTree and Grease without any failures, so this is a real odd one .... well, I'll be spending full time on these guys until I've figured them out ... so we will see what I find out ... |
I think that there is a chance, but it won't make it into 3.5.x and it may or may not make it into 3.6.x --- which is why I've been working on Rowan pretty exclusively .... we've got a customer that would like some 3.6.0-only features this fall and that is putting a mighty squeeze on me:) |
I understand that this is not going to be implemented very soon, but I wanted to understand if it would be feasible to do so in the future. I will then track the permanent solution that remove the workaround code in a future release for a future version of GemStone ;) |
…e GsProcess environment, breaking dynamic variables (#1201) * Preserve GSProcess.environment across restore of partial continuation (fixes issue #1198) * Fix for workaround in GRGemStonePlatform>>seasideSupendFlowDo: such that we only try to overwrite the environment when control is returned here as the result of restoring the partial continuation. * Fix functional tests for running in Gemstone + add workaround fix to the partial continuation tests * Converted use of Dictionary instance to use of a proper class (WAGemStoneProcessEnvironmentWrapper) in the implementation for the workaround for github issue 1198. Added a test for the specific case. * do not use latestMetacello setting for gemstone builds
@dassi I am closing this as I went ahead and merged this into master. A new Seaside version will be released soon, as I fix some more issues. If you find the opportunity to do some testing of the fix in your app, that would always be appreciated. |
@jbrichau keep an eye out for internal bug 48863 "Restore of partial continuations does not preserve GsProcess environment, breaking dynamic variables" |
Bug summary
WAFlowFunctionalTest
fails: ExceptionWARequestContextNotFound
after seaside's call/answerSteps
WARequestContextNotFound
That simply means, that seaside's very basic call:/answer is not working.
My observations
call:onAnswer:
instead ofcall:
.WADynamicVariable
(whichWACurrentRequestContext
is a subclass of). On GemStone this is now a subclass of GemStone's ownDynamicVariable
class, which in turn usesProcessor activeProcess environment at:
to get the value of the dynamic variable. I managed to figure out, that the "Processor activeProcess" changes after call:/answer, which could be OK but the process' environment dictionary is blank and the current request context is gone.The text was updated successfully, but these errors were encountered: