BrowserRequest loses its default skin on retry

[Brian Sutherland]

When the request is retried in zope.publisher.publish, a new request is created using request.retry(). This new request does not have the original default skin set.

[Maciej Wisniowski (pigletto-gmail)]

It is possibly connected with: https://bugs.launchpad.net/zope3/+bug/111845 as both problems arise when new request is created in retry()

[Christian Theune (ctheune)]

I don't think it is the same issue as the mentioned other bug. The other bug says that a retry does not occur (at least that's how I understand that bug), this bug says that when a retry occurs the request isn't identical.

[Christian Theune (ctheune)]

I can't figure out whether this really happens, a test case would be nice. Reading the code I can't tell. Anyway, this would be critical IMHO.

[Maciej Wisniowski (pigletto-gmail)]

Please take a look at (this is a link included in my bug submission mentioned in a comment above):
http://mail.zope.org/pipermail/zope3-dev/2006-July/019814.html

I understand this that new request object created in 'publish' method, while 'Retrying', is not complete (I don't know exactly why).
Because of this, Zope fails with Not found error. I suppose that this incompletness may be also a cause of the bug submitted here: 'new request doesn't have original default skin set', but it is only a suspicion...

[Launchpad Janitor (janitor)]

[Expired for Zope 3 3.2 because there has been no activity for 60 days.]

[Launchpad Janitor (janitor)]

[Expired for Zope 3 3.3 because there has been no activity for 60 days.]

[Launchpad Janitor (janitor)]

[Expired for Zope 3 3.4 because there has been no activity for 60 days.]

[Thomas Herve (therve)]

I have been hit by this bug. I have spotted the problem because HTTPPublicationRequestFactory calls setDefaultSkin, whereas the retry logic doesn't. The attached patch fixes the problem for me, although I'm not sure this is valid approach. For now, I'll add an explicit setDefaultSkin in my publication logic.

[Christophe Combelles (ccomb)]

I'm writing a lowlevel test that (at least) checks that every interface is kept is the retried request.
The fix is either to re-provide the same interfaces as the original request, or to 'copy' the request (as proposed by Sven). Resetting the default skin might work in most cases but it is probably not enough.

[Christophe Combelles (ccomb)]

fix commited in revision 89765
The fix just restores the interfaces.

[Thomas Herve (therve)]

Thanks a lot Christophe, this fixed the issue for me.

[Marius Gedminas (mgedmin)]

Christophe, consider the case when a skin is applied from a IBeforeTraversalEvent handler in the middle of the traversal path. In that case it would be incorrect to have that skin applied at the beginning of the new traversal during a retry.

It should be sufficient in all cases to set the initial (i.e. default) skin correctly; all the other skins that were applied during traversal will be re-applied during the repeat traversal.

[Marius Gedminas (mgedmin)]

Should I reopen this bug, or create a new one, for the over-eager skin application?

[Christophe Combelles (ccomb)]

Actually you're right, but it's hard to know the correct request to recreate, since it is created from its final state, and we don't really know its initial state before traversing. Traversal adapters could also modify the content of the request, not only its interfaces. Do you think we should use Thomas' proposal instead? Is it a problem to apply a skin twice?

[Fred Drake (fdrake)]

On Wed, Aug 27, 2008 at 4:53 PM, Christophe Combelles <email address hidden> wrote:
> Actually you're right, but it's hard to know the correct request to
> recreate, since it is created from its final state, and we don't really
> know its initial state before traversing. Traversal adapters could also
> modify the content of the request, not only its interfaces. Do you think
> we should use Thomas' proposal instead? Is it a problem to apply a skin
> twice?

Starting the request with a skin other than the configured default
skin could result in different behavior as the request is handled. In
the worst cases, it could end up with a different final skin than what
it ended with the first time.

 -Fred

--
Fred L. Drake, Jr. <fdrake at gmail.com>
"Chaos is the score upon which reality is written." --Henry Miller

[Christophe Combelles (ccomb)]

reopening

[Christophe Combelles (ccomb)]

Commited in rev 90561

I've used Thomas' proposal, and modified the test accordingly.
Please confirm this is OK, so that I can merge to the trunk, and release yet another zope.publisher for the imminent zope 3.4.0 final release

[Christophe Combelles (ccomb)]

released in 3.4.6