Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

about race condition issue #14

Closed
roadmap001 opened this issue Oct 13, 2012 · 5 comments
Closed

about race condition issue #14

roadmap001 opened this issue Oct 13, 2012 · 5 comments

Comments

@roadmap001
Copy link

I dont think "this should be incredibly unlikely". My app throws that error because using iframe in homepage. so maybe adding a configuration parameter is a good idea in order to reduce code dependency.

@jcoleman
Copy link
Owner

As I've been thinking about this, I could fairly easily add an option that would cause immediately flushing every time a session key is changed (or the session is created etc.) As far as I can tell, this would fix your problem (though obviously lessening performance to some degree--but this is the necessary tradeoff for a higher guarantee of synchronization.)

I'm not going to be able to work on it this week, but perhaps the following week I'll be able to get something knocked out.

@roadmap001
Copy link
Author

yeah, I changed the source code for my temporal use with the same idea of you. I think the final solution should:

  1. dont influence the upper application logic. that means the application code should ignore the redis-session-manager exists( so codes related to redis such as session.setAttribute("changed"); for app coding is not a good idea i think ).
  2. to store into redis directly for every session change is the correct direction I think, just as you said, tradeoff should be taken into consideration. but how to solve that, maybe think about it on the lower layer.

thanks

@jcoleman
Copy link
Owner

I'm going to keep the session.setAttribute("__changed__"); feature. It's admittedly a bit of a hack, but there's no good way for a user to access the actual RedisSession instance since so many frameworks (and ever servlet containers like Tomcat) insert their own arbitrary proxy wrappers around the original session instance to be able to provide their own features. Since there's no way to have those proxies forward arbitrary method calls, I'm stuck with hijacking an existing method call.

@roadmap001
Copy link
Author

umm.. what I mean is: if I code java web app, I dont care about the lower layer supporting technology. for example, I should not care about what web server I use (such as tomcat, weblogic, etc). especially in my upper app codes. so ideally, My app should run on traditional tomcat server or on redis supported tomcat server. to solve that hiding from app. Am I right?

@jcoleman
Copy link
Owner

In general, that should be true. But it's also true that underlying "support" technologies will occasionally mean you have to understand inherent limits in the underlying technology. The classic example would be that while you should be able to use any SQL server, it's actually the case that all of the options have things that are supported that aren't supported in the other options. The session.setAttribute("__changed__"); isn't something that has to be used by everyone anyway, it's a workaround for a specific use case should someone desire that level of control. If you'd like that level of control, it implies that you're already having to think through the underlying technology in use. There are inherent limits and benefits to switching to a Redis-backed session store as opposed to having it in memory. There's no way to work around those differences, it's just inherent to the technology in use.

I've tried to hide 99% of use cases from the app; for those rare occasions where you're doing something that doesn't fit it's only makes sense that you'd have to workaround it more manually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants