I tried passing the return value of session() into a function in another module which purposely does not "use Dancer". Thus, I cannot use the global DSL functions without explicit qualification. This other module modifies the session object, so the code looked something like this: package App::Helper; sub do_something_interesting { my ($session) = @_; $session->{foo} = 'bar'; # ... and lots more like that } To my surprise, I found that in the calling route handler... get '/myroute' => sub { do_something_interesting(session()); session 'more' => 'changes here'; } ...the changes to the session object made by the helper module got lost. I finally tracked it down to the fact that session(X, Y) automatically calls flush() on the object for you, whereas session() with no parameters returns a hashref that doesn't auto-flush itself. On one level, I guess live and learn. But, is it good design for Dancer to simply forget all my changes? Why doesn't the second line in my route handler simply continue to modify the in-memory session object, and flush all the changes at once?
On Thu, Feb 13, 2014 at 11:37 PM, Warren Young <warren@etr-usa.com> wrote:
I tried passing the return value of session() into a function in another module which purposely does not "use Dancer". Thus, I cannot use the global DSL functions without explicit qualification.
This other module modifies the session object, so the code looked something like this:
package App::Helper;
sub do_something_interesting { my ($session) = @_;
$session->{foo} = 'bar'; # ... and lots more like that }
To my surprise, I found that in the calling route handler...
get '/myroute' => sub { do_something_interesting(session());
session 'more' => 'changes here'; }
...the changes to the session object made by the helper module got lost.
I finally tracked it down to the fact that session(X, Y) automatically calls flush() on the object for you, whereas session() with no parameters returns a hashref that doesn't auto-flush itself.
On one level, I guess live and learn. But, is it good design for Dancer to simply forget all my changes? Why doesn't the second line in my route handler simply continue to modify the in-memory session object, and flush all the changes at once?
It does not make sense to me that calling session() as a getter (as opposed to a setter) should write out the session on each access. To do that would probably require using Tie::Hash to have it invoke flush() on every modification of the hash. I'm not sure if that extra complexity is worth the benefit. -Naveed Massjouni
On Fri, 14 Feb 2014, Naveed Massjouni wrote:
On Thu, Feb 13, 2014 at 11:37 PM, Warren Young <warren@etr-usa.com> wrote: [...]
$session->{foo} = 'bar';
session 'more' => 'changes here';
[...] It does not make sense to me that calling session() as a getter (as opposed to a setter) should write out the session on each access. To do that would probably require using Tie::Hash to have it invoke flush() on every modification of the hash. I'm not sure if that extra complexity is worth the benefit.
That's not the problem. As I read it: my $foo = session 'foo'; # getter after setter session 'more' => 'changes here'; is gone. -- Henk
On Thu, Feb 13, 2014 at 11:37 PM, Warren Young <warren@etr-usa.com> wrote:
I finally tracked it down to the fact that session(X, Y) automatically calls flush() on the object for you, whereas session() with no parameters returns a hashref that doesn't auto-flush itself.
Really, session() with no parameters returns a session *object*, it happens to be implemented as a hash ref. There's no magic to it. To make objects do things, you have to call methods on them. Weirdly, D1 doesn't implement get/set methods on the session object. Instead, Dancer::Session has read/write methods that set values directly on the session object.
On one level, I guess live and learn. But, is it good design for Dancer to simply forget all my changes? Why doesn't the second line in my route handler simply continue to modify the in-memory session object, and flush all the changes at once?
I assume you're talking Dancer 1, here, right? You've discovered one of the flaws of the D1 session model -- every write to the session via the DSL flushes the session to storage. That means going to network or disk for *every* session variable change. Ouch! What it sounds like you'd really like is to have all changes flushed when the request is finishing (even on error) and D1 doesn't do that. That said, you have two options: * Have your helper just call Dancer::Session->read() and ->write() * Save your changes yourself by calling $session->flush -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On 2/14/2014 03:45, David Golden wrote:
On Thu, Feb 13, 2014 at 11:37 PM, Warren Young <warren@etr-usa.com> wrote:
I finally tracked it down to the fact that session(X, Y) automatically calls flush() on the object for you, whereas session() with no parameters returns a hashref that doesn't auto-flush itself.
Really, session() with no parameters returns a session *object*, it happens to be implemented as a hash ref. There's no magic to it. To make objects do things, you have to call methods on them.
...But there *could* be magic to it. :) I tracked the problem down further. It is that when you call Dancer::Session::YAML::retrieve() indirectly via Dancer::session(), it is loading the session object back up from disk, obliterating the contents of the previous object. You can see the problem easily: 1. Create a new Dancer 1 project 2. Edit the lone route handler: session a => 1; session()->{b} = 2; session c => 3; The 'b' line simulates my passing of session()'s return to another module. 3. Add session: "YAML" to config.yml 4. Run the app, hit the page, examine the session file: --- !!perl/hash:Dancer::Session::YAML a: 1 c: 3 id: 375432505278121443472247271047126129 My notion was that session() was returning the same global object, in the same way that request() does. The 'b' field would remain in memory only until the 'c' field is written, which implicitly calls flush() due to the difference in the way session() handles parameters. It would be even nicer if, at the end of a route handler's processing, Dancer would detect that there is a dirty session object and flush it for you. "Exit without saving" is never a desired use case with session objects, as it can be for, say, word processing documents. This doesn't happen with Dancer::Session::Simple for fairly obvious reasons. However, I suspect it would happen with any other persistent session mechanism. The "magic" I hinted at above would be to keep the session object loaded and in RAM from the first session() call until the end of the route handler, never reloading from disk, but auto-flushing at the end. This would have the nice side benefit of speeding up session I/O. Really, there's not a lot of point of persisting the session object more than once per route call, is there? There's even less point to repeatedly re-loading it from disk. It's not like there's another process also writing to the same session. ...Or is there? With a forking front end, do all calls from a particular client go to a particular Dancer app instance, or is that parallelized, too?
I assume you're talking Dancer 1, here, right?
Yes, and I realize it's feature-frozen. This behavior strikes me as bug-like, so I don't see that the freeze is a reason not to improve the behavior.
On Fri, Feb 14, 2014 at 1:41 PM, Warren Young <warren@etr-usa.com> wrote:
session()->{b} = 2;
I agree in principle with most everything you say, except for this above. You're violating encapsulation by modifying the internals of the object. As soon as you do that, you break the API contract and so, yes, mayhem results. The fact that the object doesn't give you a useful API for direct modification is a flaw in the API, but going around it and expecting it to just "magically work" isn't really right. I would have no objection to having get/set methods added to Dancer::Session::Abstract to allow you to modify keys. At the moment, modification would need to also trigger a flush. Or, the API needs to document that you need to flush as soon as you're done modifying. I'd also be happy to see work done to defer flushing until after the request is handled (as D2 does). But someone has to do that work. And that might have unintentional side effects, I'd say that the eager flushing when setting variables might have been an intentional compensation for the race condition inherent in session loading/saving. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On Fri, Feb 14, 2014 at 2:26 PM, David Golden <xdg@xdg.me> wrote:
On Fri, Feb 14, 2014 at 1:41 PM, Warren Young <warren@etr-usa.com> wrote:
session()->{b} = 2;
I agree in principle with most everything you say, except for this above. You're violating encapsulation by modifying the internals of the object. As soon as you do that, you break the API contract and so, yes, mayhem results.
The fact that the object doesn't give you a useful API for direct modification is a flaw in the API, but going around it and expecting it to just "magically work" isn't really right.
I would have no objection to having get/set methods added to Dancer::Session::Abstract to allow you to modify keys. At the moment, modification would need to also trigger a flush. Or, the API needs to document that you need to flush as soon as you're done modifying.
Doesn't D1's Session interface already provide get/set via Dancer::Session::read/write: https://metacpan.org/source/YANICK/Dancer-1.3121/lib/Dancer/Session.pm#L53 And it seems to automatically do a flush after a write (unless is_lazy is set). -Naveed
I'd also be happy to see work done to defer flushing until after the request is handled (as D2 does). But someone has to do that work.
And that might have unintentional side effects, I'd say that the eager flushing when setting variables might have been an intentional compensation for the race condition inherent in session loading/saving.
David
-- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg _______________________________________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
On Fri, Feb 14, 2014 at 2:35 PM, Naveed Massjouni <naveed@vt.edu> wrote:
Doesn't D1's Session interface already provide get/set via Dancer::Session::read/write: https://metacpan.org/source/YANICK/Dancer-1.3121/lib/Dancer/Session.pm#L53 And it seems to automatically do a flush after a write (unless is_lazy is set).
Dancer::Session does, but Dancer::Session::Abstract does not. The first is a public API using class methods. The second is the base class for session objects -- which are what you get when you call the session() DSL without arguments. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On 2/14/2014 12:26, David Golden wrote:
I'd say that the eager flushing when setting variables might have been an intentional compensation for the race condition inherent in session loading/saving.
So it is true that if you run a Dancer app under, say, Starman, that multiple simultaneous requests to the app for the same session could go to different app forks, and thus you'd have the potential for one session to roll back another fork's changes? I also notice that neither D1 nor D2 has anything like Session::lock() to do a file lock on the session data. (Same idea as SQL transactions.)
participants (5)
-
David Golden -
Henk van Oers -
Naveed Massjouni -
Naveed Massjouni -
Warren Young