Stefan, Cemo, David, I agree that it would be a good idea to rewrite the session management afresh. There's code in there that has been there since jetty-6, maybe even longer! This isn't going to happen overnight - as with everything to do with the servlet spec its a complex beastie that requires some careful thought, and needs to take account of the many different flavours of session management: persistent/non-persistent, clustered/non-clustered, and the various technologies: jdbc, mongo etc etc etc.
The session management code should mostly hold sync locks on a specific session, rarely over the entire session manager (which will affect all session ops for a context) or over the entire session id manager (which can affect all session ops for a server). If you would like to pinpoint areas where we can look at optimising the locking strategy (patches are always nice! - see how to contribute here: http://www.eclipse.org/jetty/documentation/current/advanced-contributing.html), then that could probably be done fairly quickly. regards Jan On 27 February 2014 21:50, Stefan Magnus Landrø <[email protected]> wrote: > Hi there, > > the problem here isn't really the synchronized block itself. The problem is > that the different synchronization blocks cover lots of remote sql calls (@ > let's say 10ms each). Do the math - you'll notice how few session related > operations you can do per second per jvm ... > In my opinion, the jdbc-/(abstract-)session(id)manager code needs a complete > rewrite. > > Stefan > > > 2014-02-27 8:48 GMT+01:00 "" <[email protected]>: > >> Indeed, tomcat uses a concurrent hash map and no explicit synchronisation >> when accessing/updating session attributes. >> >> And in fact tomcat only uses synchronized in two places, when expiring the >> session and when fire events to session listeners. >> >> Yeah, I hear that synchronisation "is not such an overhead", but then put >> large load on big multi socket multicore box and watch it completely fail to >> scale to the available hardware. >> >> >> On 27 February 2014 20:35, "" <[email protected]> wrote: >>> >>> >>> Ouch. Just looked at the AbstractSessionManager... Such >>> synchronisation is the tip of the iceberg. >>> >>> I wonder if there really needs to be this amount of synchronisation when >>> the order of the requests coming in that might affect a session can be quote >>> arbitrary in themselves. On AbstractSessionManager synchronisation >>> protection on _attributes is perhaps overkill when a ConcurrentHashMap could >>> be used instead. >>> >>> >>> >>> >>> >>> On 27 February 2014 20:02, "" <[email protected]> wrote: >>>> >>>> I don't believe the synchronised block is useful here. Synchronised >>>> should be used to stop things breaking, not to protect against something >>>> that is occasionally inefficient. >>>> >>>> So this is sufficient and will avoid inconsistent synchronization >>>> warnings... >>>> >>>> @Override >>>> >>>> >>>> >>>> public void removeAttribute(String name) { >>>> >>>> >>>> >>>> >>>> if(getAttribute(name)){ >>>> super.removeAttribute(name); >>>> _dirty = true; >>>> } >>>> >>>> >>>> >>>> >>>> } >>>> >>>> >>>> >>>> >>>> On 27 February 2014 10:40, Jan Bartel <[email protected]> wrote: >>>>> >>>>> Cemo, >>>>> >>>>> This will need some refactoring of the implementation of the session >>>>> interface, but I'm not opposed to looking into it. >>>>> >>>>> Can you open a bug for this please and assign to me? >>>>> >>>>> thanks >>>>> Jan >>>>> >>>>> On 27 February 2014 06:25, Cemo <[email protected]> wrote: >>>>> > Hi, >>>>> > >>>>> > Current org.eclipse.jetty.server.session.JDBCSessionManager has >>>>> > removeAttribute as this: >>>>> > >>>>> > public void removeAttribute (String name) >>>>> > >>>>> > { >>>>> > super.removeAttribute(name); >>>>> > >>>>> > >>>>> > >>>>> > _dirty=true; >>>>> > } >>>>> > >>>>> > >>>>> > This has a side effect because of making dirty for every removing >>>>> > operation. >>>>> > This is causing a lot of trouble because each jstl set ( c:set ) call >>>>> > in JSP >>>>> > pages is also calling removeAttribute internally. What I am >>>>> > suggesting is >>>>> > that something like this: >>>>> > >>>>> > >>>>> > @Override >>>>> > public void removeAttribute(String name) { >>>>> > >>>>> > >>>>> > >>>>> > synchronized (this){ >>>>> > Object attribute = getAttribute(name); >>>>> > >>>>> > >>>>> > >>>>> > if(attribute != null){ >>>>> > >>>>> > >>>>> > >>>>> > super.removeAttribute(name); >>>>> > >>>>> > >>>>> > >>>>> > _dirty = true; >>>>> > } >>>>> > } >>>>> > } >>>>> > >>>>> > public >>>>> > >>>>> > >>>>> > https://gist.github.com/cemo/9236abe34d2b126242ad >>>>> > >>>>> > What do you think? >>>>> > >>>>> > _______________________________________________ >>>>> > jetty-users mailing list >>>>> > [email protected] >>>>> > https://dev.eclipse.org/mailman/listinfo/jetty-users >>>>> > >>>>> >>>>> >>>>> >>>>> -- >>>>> Jan Bartel <[email protected]> >>>>> www.webtide.com >>>>> 'Expert Jetty/CometD developer,production,operations advice' >>>>> _______________________________________________ >>>>> jetty-users mailing list >>>>> [email protected] >>>>> https://dev.eclipse.org/mailman/listinfo/jetty-users >>>> >>>> >>> >> >> >> _______________________________________________ >> jetty-users mailing list >> [email protected] >> https://dev.eclipse.org/mailman/listinfo/jetty-users >> > > > > -- > BEKK Open > http://open.bekk.no > > TesTcl - a unit test framework for iRules > http://testcl.com > > _______________________________________________ > jetty-users mailing list > [email protected] > https://dev.eclipse.org/mailman/listinfo/jetty-users > -- Jan Bartel <[email protected]> www.webtide.com 'Expert Jetty/CometD developer,production,operations advice' _______________________________________________ jetty-users mailing list [email protected] https://dev.eclipse.org/mailman/listinfo/jetty-users
