On Mon, 2018-08-06 at 23:00 -0400, Mark Mielke wrote: > Hi all: > > I have code that relies on BasicCookieStore being Serializable. After > updating from HttpClient 4.5.3 to 4.5.6 I found that this code was > broken. > > The ObjectInputStream.readObject() and > ObjectOutputStream.writeObject() > continue to work, but any code that accesses the cookies fails with > an > error like: > > Exception in thread "main" java.lang.NullPointerException > at > org.apache.http.impl.client.BasicCookieStore.getCookies(BasicCookieSt > ore.java:116) > > Looking at this change, which was introduced into HttpClient 4.5.4: > > https://github.com/apache/httpcomponents-client/pull/81/commits/c6413 > a01ddcfcde47c0008dca89d076cb2fe5176 > > I believe the issue is that a new "lock" variable was introduced, but > the > "lock" variable is not Serializable in its current form. When the > object is > reconstructed as a result of ObjectInputStream.readObject(), the > "lock" > variable is being left as null. Then, code like this: > > public List<Cookie> getCookies() { lock.readLock().lock(); try { > //create defensive copy so it won't be concurrently modified return > new > ArrayList<>(cookies); } finally { lock.readLock().unlock(); } } > > Is failing, because "lock" is null, and "lock.readLock()" therefore > returns > a NullPointerException. > > Another problem here, is that the serialVersionUID has not been > updated, so > this allows the new object layout and the old object layout to be > treated > as interchangeable by the ObjectInputStream, preventing it from > detecting > the conflict in layout. > > I am considering working around this, by serializing the List<Cookie> > instead of the BasicCookieStore, but I think this could affect others > and > it should be fixed. > > I think the options here include: > > 1. Revert this change. It isn't clear to me that this change > provides > significant value. The "synchronized" vs "ReadWriteLock" only > provides > value under significant load. Is the extra complexity really worth > it? > 2. Fix the code, by declaring "lock" as transient, and providing > readObject and writeObject() methods to deal with this non- > Serializable > field.
I cannot really say how justified it is but other people wanted it and were willing to provide a patch. I suggest that we keep things as is but fix the serialization problem. Cheers Oleg > 3. Fix the code by dynamically allocating a "lock" if it is found > to be > null. > > Personally, I would like to see the case for using a ReadWriteLock > here, > and if the case is not good - KISS is a good principle, and a revert > would > be the right starting point. > > What do you think? > > Once you provide feedback, I'm willing to implement the change. > > Thanks! > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
