Initial Request Failure on Retry

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Initial Request Failure on Retry

Ken Treis
We have build something using the Seaside-Rest add-on, and everything works beautifully in GemStone until a transaction conflict triggers a retry -- at which point we get a 404 rather than the expected response.

We tracked the issue down to the WAPathConsumer, which keeps track of the path (destructively) as the request is being resolved. Its state isn't being reset before the request is retried, so subsequent requests are effectively tried against a different path.

We've fixed in our home-grown Zinc streaming adaptor by adding the following line just before we retry:

> aRequestContext consumer initializeWith: aRequestContext request url path copy


This is admittedly a dreadful encapsulation violation. Ideally, maybe we'd implement something like #reset on WARequestContext. There is some other internal state (like `handlers`) that should probably be reset too, though I think in practice the use of #ensure: in #push:during: empties out the handlers collection before handling is retried.

Alternately, we could perhaps make WAPathConsumer non-destructive and implement #reset there. It already acts a bit like a stream, so a #reset method would not be too out of place there.

Thoughts?

--
Ken Treis
Miriam Technologies, Inc.

_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Initial Request Failure on Retry

Julian Fitzell-2
Abstractly—and I admit I haven't gone to look at the code—I don't see a problem with having #reset on WAPathConsumer, since as you say it is stream-like. But wouldn't it be simpler just to create a new request context when you retry?

On Thu, Mar 14, 2013 at 6:12 PM, Ken Treis <[hidden email]> wrote:
We have build something using the Seaside-Rest add-on, and everything works beautifully in GemStone until a transaction conflict triggers a retry -- at which point we get a 404 rather than the expected response.

We tracked the issue down to the WAPathConsumer, which keeps track of the path (destructively) as the request is being resolved. Its state isn't being reset before the request is retried, so subsequent requests are effectively tried against a different path.

We've fixed in our home-grown Zinc streaming adaptor by adding the following line just before we retry:

> aRequestContext consumer initializeWith: aRequestContext request url path copy


This is admittedly a dreadful encapsulation violation. Ideally, maybe we'd implement something like #reset on WARequestContext. There is some other internal state (like `handlers`) that should probably be reset too, though I think in practice the use of #ensure: in #push:during: empties out the handlers collection before handling is retried.

Alternately, we could perhaps make WAPathConsumer non-destructive and implement #reset there. It already acts a bit like a stream, so a #reset method would not be too out of place there.

Thoughts?

--
Ken Treis
Miriam Technologies, Inc.

_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev


_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Initial Request Failure on Retry

Philippe Marschall
In reply to this post by Ken Treis
On Thu, Mar 14, 2013 at 7:12 PM, Ken Treis <[hidden email]> wrote:
> We have build something using the Seaside-Rest add-on, and everything works beautifully in GemStone until a transaction conflict triggers a retry -- at which point we get a 404 rather than the expected response.

Can you provide a bit more detail about the retry code? Where (server
adapter,filter, ...) does it run and what does it look like?

Thinking about it you may actually need a bit more than just resetting
the path consumer. The properties on the request context and the
response buffer as well.

Cheers
Philippe
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Initial Request Failure on Retry

Ken Treis
On Mar 16, 2013, at 2:05 AM, Philippe Marschall wrote:

On Thu, Mar 14, 2013 at 7:12 PM, Ken Treis <[hidden email]> wrote:
We have build something using the Seaside-Rest add-on, and everything works beautifully in GemStone until a transaction conflict triggers a retry -- at which point we get a 404 rather than the expected response.

Can you provide a bit more detail about the retry code? Where (server
adapter,filter, ...) does it run and what does it look like?

It's in a server adaptor that was stitched together from Dale's standard code for GemStone, a streaming hack for Zinc, and a quick back-port of your WAComboResponse for use in Seaside 3.0. Code is available at http://mc.miriamtech.com/zinc

The starting point is WAGsZincStreamingAdaptor>>handleRequest: aRequestContext

1. Spins until we get necessary GS locks.
2. Attempts to process the request
3. Tries to commit the GS transaction.
4. If commit fails, abort and attempt to process again.

Details below. This is really a GemStone-specific issue, but my life would be simpler with something like #reset or #resetIfPossible on WAPathConsumer and/or WARequestContext.

Thinking about it you may actually need a bit more than just resetting
the path consumer. The properties on the request context and the
response buffer as well.

I'm resetting the response buffer by sending #resetIfPossible, and we've hacked the path consumer reset into there, but I'm not doing anything with properties on the request context. What might need to be changed there?

Here's a quick overview of the retry-related code in WAGsZincStreamingAdaptor, with some GemStone-specific details left out or pseudo-coded for brevity.

handleRequest: aRequestContext
| timeout |
timeout := Time now addSeconds: 60.
[self handleRequestCheckingLock: aRequestContext]
whileFalse: [
Time now > timeout
ifTrue: [
|response|
response := aRequestContext response.
response internalError.
response stream nextPutAll: 'The system is too busy, please try again in a moment.'.
^self
].
(Delay forMilliseconds: 10) wait.
aRequestContext consumer initializeWith: aRequestContext request url path copy ]

(I know, the path consumer reset feels out of place here... this was just a quick hack to make things work.)

handleRequestCheckingLock: aRequestContext
"answer false to retry request"

| result retryRequest |
GRPlatform current transactionMutex critical: [
retryRequest := false.
[ super handleRequest: aRequestContext ]
on: WARetryHttpRequest
do: [:ex | retryRequest := aRequestContext response isCommitted not ].
retryRequest 
ifTrue: [
"lock not acquired - unwind the stack to this point and leave transaction mode"

GRPlatform current doAbortTransaction.
^false ].
GRPlatform current doCommitTransaction
ifFalse: [ 
GRPlatform current doAbortTransaction.
aRequestContext response resetIfPossible
ifTrue: [
GRPlatform current saveLogEntry: 'Commit failure - retrying'.
^false]
ifFalse: [
GRPlatform current saveLogEntry: 'Commit failure after response sent, giving up'.
^true]].
].
^true

My apologies for the poor code factoring/formatting.

Julian suggested creating a new context, which is something I hadn't considered but probably should...


Ken

_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev
Reply | Threaded
Open this post in threaded view
|

Re: Initial Request Failure on Retry

Philippe Marschall
On Wed, Mar 20, 2013 at 7:30 AM, Ken Treis <[hidden email]> wrote:

> On Mar 16, 2013, at 2:05 AM, Philippe Marschall wrote:
>
> On Thu, Mar 14, 2013 at 7:12 PM, Ken Treis <[hidden email]> wrote:
>
> We have build something using the Seaside-Rest add-on, and everything works
> beautifully in GemStone until a transaction conflict triggers a retry -- at
> which point we get a 404 rather than the expected response.
>
>
> Can you provide a bit more detail about the retry code? Where (server
> adapter,filter, ...) does it run and what does it look like?
>
>
> It's in a server adaptor that was stitched together from Dale's standard
> code for GemStone, a streaming hack for Zinc, and a quick back-port of your
> WAComboResponse for use in Seaside 3.0. Code is available at
> http://mc.miriamtech.com/zinc
>
> The starting point is WAGsZincStreamingAdaptor>>handleRequest:
> aRequestContext
>
> 1. Spins until we get necessary GS locks.
> 2. Attempts to process the request
> 3. Tries to commit the GS transaction.
> 4. If commit fails, abort and attempt to process again.
>
> Details below. This is really a GemStone-specific issue, but my life would
> be simpler with something like #reset or #resetIfPossible on WAPathConsumer
> and/or WARequestContext.
>
> Thinking about it you may actually need a bit more than just resetting
> the path consumer. The properties on the request context and the
> response buffer as well.
>
>
> I'm resetting the response buffer by sending #resetIfPossible, and we've
> hacked the path consumer reset into there, but I'm not doing anything with
> properties on the request context. What might need to be changed there?
>
> Here's a quick overview of the retry-related code in
> WAGsZincStreamingAdaptor, with some GemStone-specific details left out or
> pseudo-coded for brevity.
>
> handleRequest: aRequestContext
> | timeout |
> timeout := Time now addSeconds: 60.
> [self handleRequestCheckingLock: aRequestContext]
> whileFalse: [
> Time now > timeout
> ifTrue: [
> |response|
> response := aRequestContext response.
> response internalError.
> response stream nextPutAll: 'The system is too busy, please try again in a
> moment.'.
> ^self
> ].
> (Delay forMilliseconds: 10) wait.
> aRequestContext consumer initializeWith: aRequestContext request url path
> copy ]
>
> (I know, the path consumer reset feels out of place here... this was just a
> quick hack to make things work.)
>
> handleRequestCheckingLock: aRequestContext
> "answer false to retry request"
>
> | result retryRequest |
> GRPlatform current transactionMutex critical: [
> retryRequest := false.
> [ super handleRequest: aRequestContext ]
> on: WARetryHttpRequest
> do: [:ex | retryRequest := aRequestContext response isCommitted not ].
> retryRequest
> ifTrue: [
> "lock not acquired - unwind the stack to this point and leave transaction
> mode"
>
> GRPlatform current doAbortTransaction.
> ^false ].
> GRPlatform current doCommitTransaction
> ifFalse: [
> GRPlatform current doAbortTransaction.
> aRequestContext response resetIfPossible
> ifTrue: [
> GRPlatform current saveLogEntry: 'Commit failure - retrying'.
> ^false]
> ifFalse: [
> GRPlatform current saveLogEntry: 'Commit failure after response sent, giving
> up'.
> ^true]].
> ].
> ^true
>
> My apologies for the poor code factoring/formatting.
>
> Julian suggested creating a new context, which is something I hadn't
> considered but probably should...

Yes, this sounds like a better approach.

Cheers
Philippe
_______________________________________________
seaside-dev mailing list
[hidden email]
http://lists.squeakfoundation.org/mailman/listinfo/seaside-dev