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(BasicCookieStore.java:116) Looking at this change, which was introduced into HttpClient 4.5.4: https://github.com/apache/httpcomponents-client/pull/81/commits/c6413a01ddcfcde47c0008dca89d076cb2fe5176 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. 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! -- Mark Mielke <[email protected]>
