Bug 408167

Comment 1 Jan Bartel 2013-05-17 03:21:17 EDT

Fixed for jetty-9.0.4. Not only applies to simple types, but all attribute values.

Jan

Comment 2 Christoph Laeubrich 2013-05-17 03:32:36 EDT

Just wondering, how do you check this for complex types (e.g. custom objects)?

Comment 3 Jan Bartel 2013-05-17 03:40:34 EDT

Laeubi,

I just hooked into the existing code that determines if any of the attribute listeners etc need to be called, depending on if the value had changed or not:

see line 442 of AbstractSession:

 protected boolean updateAttribute (String name, Object value)
    {
        Object old=null;
        synchronized (this)
        {
            checkValid();
            old=doPutOrRemove(name,value);
        }

        if (value==null || !value.equals(old))
        {
            if (old!=null)
                unbindValue(name,old);
            if (value!=null)
                bindValue(name,value);

            _manager.doSessionAttributeListeners(this,name,old,value);
            return true;
        }
        return false;
    }

this is called from AbstractSession.setAttribute(), and from JDBCSessionManager.Sesssion's overridden setAttribute() method, which uses the return value of udpateAttribute to set the dirty flag:

  @Override
        public void setAttribute (String name, Object value)
        {
            _dirty=updateAttribute(name, value);
        }

Comment 4 Christoph Laeubrich 2013-05-17 03:50:36 EDT

Okay, I'm just currious if equals would work well here, tanking an general Object into account, that inherits equals form Object, it will compare always equal to itself but might has changed one of its values:

MyObject implements Serializable {
 public int val;
}

if I do:
MyObject o = ...
session.setAttribute("test", o);

And in another request:
MyObject o = (MyObject)session.getAttribute("test");
o.val = 12345;
session.setAttribute("test", o);

Wont this caue equals to return true and value not beeing persited? I can assume that this won't matter for inmemmory actions but for persiting outside the running VM I think its not valid to assume that if two object compare equal they have not changed...

Comment 5 Jan Bartel 2013-05-17 03:58:29 EDT

Hi Laeubi,

In the example that you've given, if that field is important to the identity/state of the object, then its class should override equals() to reflect that.

regards
Jan

Comment 6 Christoph Laeubrich 2014-01-16 14:14:26 EST

Hi Jan,

I wan't to reopen the issue once again, you might close it again if you find it invalid. I debugged a session related problem that involved the JDBC manager but for the 8.1.13 Version, is it rigth that this only was applied to jetty-9?

Beside this, I think the fix is invalid for the following case:
- Assume I have an Object instance X (type does not matter), and all its hashcode and equals are implemented "right".
- I put the object into the session and it is persited.
- I now change the internal state of that object (e.g. increment a counter) that is part of the equal check
- to reflect the change i add again, exactly this instance X

What will happen:
- Because in the Map ther exists this instance X
- it will be returned as the "old" one
- since it is the identical to the one set, equal will always evaluate to true (same applies of cource if equal was not overriden)
- the session will assume that the object has not changed and will not persist the changes!

I think the only valid choices here are
a) always mark session dirty on setAttribute
b) only assume not to change the dirty flag if a primitive (int/Integer/long/Long...) or string is set.
c) test if the old object is == the new object (since wen then can't tell if state has changed easily)

Comment 7 Jan Bartel 2014-01-17 02:47:26 EST

laeubi,

I think you're right about the object not being persisted if it is the same object with a changed value.

I'm considering a change that will be along the lines you suggest. For the test to see if it is a "simple" type and the same value and therefore not set the dirty flag (unless some other attribute has already set it), I'm considering this code:

 if (old != null  && value != null && old.getClass() == value.getClass() && (value instanceof Number || value instanceof String) && old.equals(value)) 

Do you think that meets the requirements? Do you have any other suggestions?

Jan


(In reply to laeubi from comment #6)
> Hi Jan,
> 
> I wan't to reopen the issue once again, you might close it again if you find
> it invalid. I debugged a session related problem that involved the JDBC
> manager but for the 8.1.13 Version, is it rigth that this only was applied
> to jetty-9?
> 
> Beside this, I think the fix is invalid for the following case:
> - Assume I have an Object instance X (type does not matter), and all its
> hashcode and equals are implemented "right".
> - I put the object into the session and it is persited.
> - I now change the internal state of that object (e.g. increment a counter)
> that is part of the equal check
> - to reflect the change i add again, exactly this instance X
> 
> What will happen:
> - Because in the Map ther exists this instance X
> - it will be returned as the "old" one
> - since it is the identical to the one set, equal will always evaluate to
> true (same applies of cource if equal was not overriden)
> - the session will assume that the object has not changed and will not
> persist the changes!
> 
> I think the only valid choices here are
> a) always mark session dirty on setAttribute
> b) only assume not to change the dirty flag if a primitive
> (int/Integer/long/Long...) or string is set.
> c) test if the old object is == the new object (since wen then can't tell if
> state has changed easily)

Comment 8 Christoph Laeubrich 2014-01-17 04:11:01 EST

I would suggest to split this into dedicated methos for complexer boolean expressions, I currently came out with this very defensive aproach:

....

if (needsUpdate(value, old))  {

....


 /**
     * Guess by best effort if a value has changed and needs an update
     * 
     * @param newValue
     *            the new value to compare
     * @param oldValue
     *            the old value to compare
     * @return <code>true</code> if values are not interchangeable and needs
     *         update, <code>false</code> otherwhise
     */
    private static boolean needsUpdate(Object newValue, Object oldValue) {
        //check if both are null, then no update required...
        if (newValue == null && oldValue == null) {
            return false;
        }
        // new value is null, but old not, its a removal of an old not null value...
        if (newValue == null) {
            return true;
        }
        //Check if it is one of the save java build-in types
        Class<? extends Object> clazz = newValue.getClass();
        if (clazz.isPrimitive() || clazz.isEnum() || isWrapperType(clazz) || isSaveType(clazz)) {
            //primitive types and Strings are safe to compare...
            return newValue.equals(oldValue);
        }
        //we don't know.. so be save and update the value
        return true;
    }

    private static boolean isSaveType(Class<? extends Object> clazz) {
        return clazz.equals(String.class) || //Strings are save...
                clazz.equals(BigDecimal.class) || // BigDecimal and Integer are also save but only from an API point of view 
                clazz.equals(BigInteger.class);
    }

    private static boolean isWrapperType(Class<?> clazz) {
        return clazz.equals(Boolean.class) || // bool
                clazz.equals(Character.class) || // char
                clazz.equals(Byte.class) || // byte
                clazz.equals(Short.class) || //  short
                clazz.equals(Integer.class) || // int
                clazz.equals(Long.class) || // long
                clazz.equals(Float.class) || // float
                clazz.equals(Double.class) || // double
                clazz.equals(Void.class); // void
    }

This will only considder "save" Objects, any custom object is assumed always changed. Then after thinking a bit more about this, I came to the conclusin that even if they are NOT the same instance, each one (old and new) might be modified outside the scope of session. They only save way would be to seriallize/deseriallize each object to see if state changed, thats IMO a little bit to much.

If someone really is concerned about custom objects updates he better check this in the application level (since there he ultimativly knows whats going on), or only use primitive types.

Just one side-note: setAttribute should check if the Object is really Seriallizable or null and maybe throw an IllegalArgumentException (not sure if this is allowed from the spec), this would prevent putting invalid Objects into the session that later can't be seriallized.

Comment 9 Christoph Laeubrich 2014-01-17 04:12:24 EST

this
 return newValue.equals(oldValue);

myst be 
 return !newValue.equals(oldValue);

of course, sorry for the typo!

Comment 10 Jan Bartel 2014-01-17 16:54:47 EST

laeubi,

Thats quite a performance penalty on every call to setAttribute. I'm not convinced its worth it when the edge case we're trying to address arises from some pathological code that sets and resets the same String value onto a session - the proper course of action is to fix the pathological code (it just so happens in your case that you can't). I'm inclined to rollback the original fix and simply call dirty=true every time setAttribute is called.

Jan

Comment 11 Christoph Laeubrich 2014-01-18 04:16:40 EST

Hi jan,

even if the check is quite cheap compared to an DB update I see your point. Maybe you want to considder adding an option (checkForChanges or something) to simply switch this on and off?
So ppl can decide if they want to pay the price for additional checking.

I agree that ideally it should be fixed in the calling code. The problem is if it is inside a lib. Since the code ios there it won't hurt much to switch it of by default and allow an option to switch it on for such cases.

Comment 12 Jan Bartel 2014-01-19 18:47:56 EST

Hi laeubi,

On consideration, I think this is behaviour that is probably best implemented in your particular environment. 

So, for you to be able to achieve that, I've made the JDBCSessionManager.Session  class subclassable, so you should now be able to implement any checks you like in a derived JDBCSessionManager.Session.setAttribute() before setting the dirty flag.

The default behaviour is back to how it was before I implemented the (faulty) change to try and not set the dirty flag if the object was the same. In other words, the default behaviour is back to calling dirty=true if you call setAttribute.

Here's an example of how to do the subclassing:

  public static class MyJDBCSessionManager extends JDBCSessionManager
    {

        public class MyJDBCSession extends JDBCSessionManager.Session
        {

            protected MyJDBCSession(HttpServletRequest request)
            {
                super(request);
            }

            protected MyJDBCSession (String sessionId, String rowId, long created, long accessed, long maxInterval)
            {
                super(sessionId, rowId, created, accessed, maxInterval);
            }

            @Override
            public void setAttribute(String name, Object value)
            {
                System.err.println("Doing my own setAttribute: "+name+"="+value);
                updateAttribute(name, value);
                _dirty = true;
            }
        }

        @Override
        protected AbstractSession newSession(HttpServletRequest request)
        {
            return new MyJDBCSession(request);
        }

        @Override
        protected AbstractSession newSession(String sessionId, String rowId, long created, long accessed, long maxInterval)
        {
            return new MyJDBCSession(sessionId, rowId, created, accessed, maxInterval);
        }
    }

I'm going to call this one closed now, but please reopen if my changes don't give you all the flexibility you need to subclass and override.

cheers
Jan

Comment 13 Christoph Laeubrich 2014-01-20 02:10:21 EST

Hi Jan,

I think this is also a good solution, thanks.