kemalcr / kemal-session

Simple session handler for Kemal
MIT License
53 stars 30 forks source link

File session can die if file removed #45

Open rdp opened 7 years ago

rdp commented 7 years ago

set the

   config.timeout = Time::Span.new(days: 0, hours: 0, minutes: 10, seconds: 0)

then access your web server so the cookie file is created.

Now wait one minute so the File.utime stuff will be triggered (or possibly until a gc fires that cleans it up?)

Delete the file.

Access your web server.

Error setting time to file './sessions/58c9d216c8ba7334f0bd875798feb61f.json': No such file or directory (Errno) 0x102681975: CallStack::unwind:Array(Pointer(Void)) at ?? 0x102681911: CallStack#initialize:Array(Pointer(Void)) at ?? 0x1026818e8: CallStack::new:CallStack at ?? 0x102656181: raise:NoReturn at ?? 0x1026df2f3: File::utime<Time, Time, String>:Nil at ?? 0x1027b06de: Session::FileEngine#is_in_cache?:Bool at ?? 0x1027b07f9: Session::FileEngine#string?<String, String>:(String | Nil) at ?? 0x1027a0eed: Session#string?:(String | Nil) at ?? 0x1026783fa: ~procProc(HTTP::Server::Context, String)@kemal_server.cr:456 at ??

though it does recover a bit after that.

I'm also not sure how I feel about the file stuff being cached at all, since the cache will persist across requests which means if it's in front of a load balancer, with a shared file system, it wouldn't persist across servers. Maybe this should be noted somewhere? Or I think ideally it would at least re-read "in" the file at the beginning of each request...or at least compare mtime with the last known mtime. Or else be made clear that it doesn't work today with shared storage (or perhaps I'm wrong?)

Cheers!

rdp commented 7 years ago

In retrospect, even if you're running two servers on the same box, and using the file backing, the two can get out of sync if they both cache the same session from the file system...so much difficulty makes me think to propose that it never cache but just always re-read from the file system. It also seems that destroy_all doesn't work because of the cache (leaves cached one undestroyed). Though unrelated...FWIW...

rdp commented 7 years ago

In addition, it's not hard to imagine a race condition scenario today, when servicing multi simultaneous requests, these lines:

          load_into_cache(session_id) unless is_in_cache?(session_id)
          return @cache.{{name.id}}(k)

could be run interleaved by different fibers, and return the value from a different session [?]

crisward commented 7 years ago

File storage also has an issue when utime fails. I have this working on a development server inside a vm. The storage folder is on the host, and doesn't seem to support changing the time.

So it throws an exception every time is_in_cache is called which I think is once a minute.

On the live server, without the vm / host drive, it works fine.

As for the issues above, a note to say file sessions are not suitable for apps with multiple instances would probably be enough. If you're app needs multiple instances, it probably also needs redis based sessions?

sdogruyol commented 7 years ago

I agree with @crisward that something like redis or memcached will be more suitable for apps with multiple instances

rdp commented 7 years ago

Yeah or mysql is a typical one. I think the original problem is still present even if run on only one instance however...

On Thu, May 4, 2017 at 5:57 AM, Serdar Dogruyol notifications@github.com wrote:

I agree with @crisward https://github.com/crisward that something like redis or memcached will be more suitable for apps with multiple instances

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/kemalcr/kemal-session/issues/45#issuecomment-299164808, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAw0OjT8A8OYTsAo1IWKWJk_FRB5LhTks5r2b0_gaJpZM4M9Suc .