redirect does't write session content to YAML file #432
Comments
|
You'll probably need to look in Flagging the response as 'halted' in redirect be not be the right thing to do. The dispatcher not calling 'after' hooks when the response is flagged as halted may be incorrect too. I'll think it over some more - hopefully a core dev can comment though! |
|
So, it works with session Simple, for instance? Given it doesn't need to flush? |
|
I confirme that it works with session Simple. |
|
Thanks, @drebolo. That should help :-) |
|
We can call the after hook always. The user should be responsible to check in the hook code if the response is halted and act accordingly. |
|
@ambs I agree; |
|
@veryrusty, yep, that is a problem. But probably can be solved with caution. |
|
I am working on this, but if someone knows how to fix it, let me know :-) |
Some modules, most notably Dancer2::Session::Cookie, require this to work. See also PerlDancer#432. Signed-off-by: Pedro Melo <melo@simplicidade.org>
|
I agree with @veryrusty and @ambs. The hook should always be called. |
|
Was working on this, still could not fix it. Will not have time during the week. Maybe during weekend. So, if someone wants to takeover, just let me know :) |
|
I just hit another plugin test failure that I've bisected to that same commit. Was there ever a clear rationale for it or some discussion I can reference? Otherwise, I'm strongly inclined to suggest it be reverted. |
|
The rationale was that if you |
|
@ambs, for what it's worth, I think part of the problem is the code in Dispatcher, but the other part is in I think it might need to check if status is 302 (or Response needs a method for |
|
@dagolden which commit was it? Was it the one that moves Session to "ro", or a different one? We've had multiple discussions on IRC, in various scattered issues (and it's noted in my release notes) about the transition to a more mutable state. It helped us clean some code and find problems in other code we weren't aware of. There is still a leak I've encountered (which I've written about in some ticket I don't remember) that is a side-effect of some of the mutable state design. Generally I'm in favor of changing everything to "ro", but that's clearly too optimistic for now. The reality is: reverting the entire commit, if necessary, can be done with no further discussion, if that's what you deem the correcting act. Just let me know which one. The top priority in my mind right now is correcting this error. I'd also be happy if we could cover in tests the problems so we could make sure we don't make the same mistake twice. "Fool me once" and all that. :) |
|
@xsawyerx I don't believe this issue is not related to 'ro' changes. This issue tracks the problem of, when you do a redirect, some hooks that the session plugins were using to store the dirty session are no longer being called. |
|
The commit that added the halt to redirect is 7452862 as @drebolo initially reported. It was part of a series of commits to make As @dagolden has already pointed, the side effects of flagging the response as halted are that 'after' hooks in the dispatcher are no longer called, the that compile_hooks also has a test for halted responses. |
|
Sorry if I sound annoyed, but why do we need to be always discussing "to revert" instead of thinking on "fixing" things? I trust Dancer developers. If the change was made, probably there was a good reason for that. And as I looked to the code, it seems possible to be fixed. Just gives a little extra work to fix code than to comment here. |
|
You're talking about "fixing forward". That's certainly one approach. I asked about rationale because if it fixed a known bug (and had unintended consequences), that's different to me than a "good idea" that had unintended consequences. Good ideas that turn out bad mean that whatever problem the good idea was trying to solve was not sufficiently understood. I'd prefer to revert such ideas and put them back on a branch until they are better understood and better tested. Consider #473. Fixing whitespace was a "good idea". Except it broke tests because templates have significant whitespace. (And was merged without testing. NB. broken CI is not a valid excuse.) In the space of a single week, I have encountered three "good ideas" that turned out bad:
With this track record, I'd be very worried that "fixing forward" is going to be more "good ideas" that aren't well thought out. You said "I trust Dancer developers". I did trust them. I'm not so sure anymore. |
|
@ambs because if you revert and thereby fix the bug for the moment, you can stop and think about how to fix it properly, rather than rushing into a new iteration of the feature that may end up simply being differently buggy. |
|
With this patch it is possible to get rid of the "halt" in case of redirect, and use only the redirect flag if you prefer. |
|
Thinking about redirect and forward semantics, forward is described as an "internal redirect" that avoids overhead of the browser making another request. From that standpoint, I think that it should behave as much like a real redirect as we can make it. Forward would then be an optimization and the only client difference is that under forward the apparent URL doesn't change. Thinking about 'is_halted', it seems like this is in the hook filter so that once one filter calls halt, no more filters will run. That makes a lot of sense for before filters; halted means the response is now final. And that would be true of both redirect and forward. But that filtering doesn't make sense for after hooks. Those should always run because they are after the response, whether the response came from a before hook or from a route. I also think that after hooks should run for forward if we want to treat them like an internal redirect. I'm not sure what it would take to implement that, but that's how I think it should work. If my statement about forward acting just like a redirect isn't what people think should happen, then I'd like a really clear explanation of the semantics of forward. |
|
Just a note: forward and redirect are similar, but if not implemented with caution, forward might have the after hook running twice, and that might not be a good idea. |
|
Why shouldn't forward run the after hook twice? A real redirect would run the after hook twice. Once on the redirect response and then again handling the redirected request from the client. This is why I say if a forward is just an optimized redirect then it should run exactly the same way hooks and all. If, however, a forward is something else — some sort of request-munging route delegator — then someone please explain very clearly the exact semantics it should have. (Not what the code does now, what it should do, so we can check it against the code and fix the semantics if wrong.) |
|
Please give a look to forward chained rules tests. It might help. |
|
On Fri, Sep 20, 2013 at 05:39:22AM -0700, Alberto Simões wrote:
The question here is what, semantically, it should do. Tests demonstrate what, technically, it does do. Currently. So by definition that can't help at all. Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN |
|
On Fri, Sep 20, 2013 at 07:16:21AM -0700, Alberto Simões wrote:
This is not about your english. First language english speakers use 'just' I'm trying to get you to expand on your thoughts such that we end up with Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN |
|
On Fri, Sep 20, 2013 at 07:22:24AM -0700, David Golden wrote:
"immediate control transfer" sounds like it's still 'goto' to me. How does like look? - When forward() is executed, the current execution process is aborted and Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN |
|
@shadowcat-mst, that seems clear enough to me. It's sort of a "redo with modified request" effect. |
|
If no one else wants to chime in, let me try to summarize:
So then I wonder about sessions and forward. I think they either need to be explicitly flushed and then recreated for the new execution, or else the session needs to be preserved across the forward redispatch. |
|
This:
@dagolden summary is complete from my POV, although I'm trying to come up with a suggestion to keep the session alive between forwards. |
|
@melo, sidetracking into technical details, it looks like the current code has the 'forward' method in the request redispatch immediately using the same context, which should preserve it. Though it doesn't look like the context is ever updated with the newly modified request, so that's possibly another bug. The problem with that approach is that it's still way down in the call stack of the original dispatch, so unless return values are handled really carefully (particularly in hooks), the original execution seems likely to resume. Throwing an exception or something with the response object to jump out of the call stack could be an ugly, but effective way to make sure the original process doesn't continue. |
|
@dagolden I didn't look at the code, but I would thrown an exception to restart the processing. I don't consider it a ugly hack. Granted, exception throwing was not designed for flow control, but it is very effective and if properly done, with blessed objects, hard to do wrong, in comparison with the alternative. |
|
On Fri, Sep 20, 2013 at 09:34:38AM -0700, David Golden wrote:
If forward is meant to jump entirely to a new route, then we can use Using an exception for this wouldn't be nearly so sane - eval frames in the Note that Return::MultiLevel is optional-XS only so this wouldn't compromise Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN |
|
/me adds new module to his toolbox. With sessions, they would be recreated, but any modifications are lost unless flushed. Building a new context and passing in the old session would save a flush/load roundtrip. |
|
@shadowcat-mst yeah, forgot about eval's... not so good a solution. Return::MultiLevel looks really nice... Thanks for the tip. |
|
Just removed my PR that fixed this YAML issue as it seems that a lot more needs to be rewritten. |
|
Seems like all the good stuff happens while I'm sleeping. @dagolden's short 'redo with modified request' nails it for me. Personally I'd When forward() is executed, the current dispatch of the request is I'm off to try out Return::MultiLevel. Thanks for that suggestion @shadowcat-mst! |
|
@veryrusty I wouldn't even say "matching a new route" as nothing requires that. Let's talk about "request" instead of routes. Clarifying "dispatch" is helpful because we actually have that method in the dispatcher, so now we've localized the semantics a bit.
|
|
On Sat, Sep 21, 2013 at 02:52:25AM -0700, Russell Jenkins wrote:
I like that better too. Nicely done.
While I don't think we need them here, Worlogog::Incident and Worlogog::Restart Matt S Trout - Shadowcat Systems - Perl consulting with a commit bit and a clue http://shadowcat.co.uk/blog/matt-s-trout/ http://twitter.com/shadowcat_mst/ Email me now on mst (at) shadowcat.co.uk and let's chat about how our CPAN |
As suggested in PerlDancer#432, use Return::MultiLevel to wrap route dispatch. Cache the multilevel return coderef in the context for forward/redirect to use. Add Return::MultiLevel to prereqs and Scope::Upper to recommends.
Use the with_return handler to return immediatly to the dispatcher after the redirect details have been added to the response. Resolves PerlDancer#432.
…r forward. This is the implementation of "... after a forward is executed, any remaining code (route and hooks) from the current dispatch will never be run." from PerlDancer#432.
As suggested in PerlDancer#432, use Return::MultiLevel to wrap route dispatch. Cache the multilevel return coderef in the context for forward/redirect to use. Add Return::MultiLevel to prereqs and Scope::Upper to recommends.
Use the with_return handler to return immediatly to the dispatcher after the redirect details have been added to the response. Resolves PerlDancer#432.
…r forward. This is the implementation of "... after a forward is executed, any remaining code (route and hooks) from the current dispatch will never be run." from PerlDancer#432.
As suggested in #432, use Return::MultiLevel to wrap route dispatch. Cache the multilevel return coderef in the context for forward/redirect to use. Add Return::MultiLevel to prereqs and Scope::Upper to recommends.
…r forward. This is the implementation of "... after a forward is executed, any remaining code (route and hooks) from the current dispatch will never be run." from #432.
|
Awesome, @sukria++ @veryrusty++ I'll test this with my apps to see if I can update from 0.08 to master now. |
|
Any news on this @melo? Can we close that one? ;) thanks! |
|
Haven't tested yet. Should be able to test it this week, tomorrow even, starting a new App, and I can try master with it. If somebody else can test this before me, thanks... |
With Dancer2-0.09 I created a test app:
package mywebapp;
use Dancer2;
use Data::Dumper;
our $VERSION = '0.1';
get '/' => sub {
session 'I am going to remain' => 'true';
warn Dumper session;
return session;
};
get '/redirect_target' => sub {
session 'I am a new value' => 'true';
warn Dumper session;
return "redirected";
};
get '/redirect_src' => sub {
session 'I am going to disappear' => 'true';
warn Dumper session;
redirect '/redirect_target';
};
true;
my config.yaml
appname: "mywebapp"
layout: "main"
charset: "UTF-8"
template: "simple"
session: YAML
as you can see in route get '/redirect_src' I have a redirect '/redirect_target';
that redirect prevents the session to write its new contents ( 'I am going to disappear' => 'true';) to YAML file.
you can confirme this in the logs.
in trying to trace this issue, I went to Core/Context
sub redirect, found a halt there, that was added wiith this 7452862. Just to test I commented the halt and the session started to write its contents to YAML file.
The text was updated successfully, but these errors were encountered: