Skip to content
This repository

Avoid a mysterious NPE #321

Closed
wants to merge 1 commit into from

2 participants

drapp Pablo Fernandez
drapp

One user on a particular device was getting an NPE here, meaning that either the header map had a null value, or it was getting mutated in a different thread deleting a key that we expected to be there. I wasn't able to reproduce this locally, but this should protect against any eventuality.

drapp protect against a mysterious null
One user on a particular device was getting an NPE here, meaning that either the header map had a null value, or it was getting mutated in a different thread deleting a key that we expected to be there. I wasn't able to reproduce this locally, but this should protect against any eventuality.
ff2d112
Pablo Fernandez

I believe the cause for this is (from Map javadocs):

The set is backed by the map, so changes to the map are reflected in the set, and vice-versa. If the map is modified while an iteration over the set is in progress (except through the iterator's own remove operation), the results of the iteration are undefined.

So technically this doesn't prevent the errors from happening but just reduces the problem window (e.g. somebody could still remove stuff from the map after the conditional but before the body of the conditional).

drapp

It won't help in the case where another thread sets the value of a key to null at exactly the wrong time. Getting the entrySet rather than the keySet should help in the case where another thread removes a header at the wrong time. The null check will also fix the case where there's just a null value in the header map to begin with, which is what I'd put my money on the problem being, since it happens consistently.

Pablo Fernandez

So the fix is not about changing to entrySet but doing the null check, right?

drapp

I have both to be extra safe, but yeah, I think the null check is what's fixing the actual bug. I'd experiment to winnow down the fix a bit, but like I said, I can't reproduce this locally so I'd have to convince the customer to keep trying different jars

Pablo Fernandez fernandezpablo85 closed this February 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 1 unique commit by 1 author.

Nov 07, 2012
drapp protect against a mysterious null
One user on a particular device was getting an NPE here, meaning that either the header map had a null value, or it was getting mutated in a different thread deleting a key that we expected to be there. I wasn't able to reproduce this locally, but this should protect against any eventuality.
ff2d112
This page is out of date. Refresh to see the latest.
8  src/main/java/org/scribe/model/Response.java
@@ -45,9 +45,11 @@ private String parseBodyContents()
45 45
   private Map<String, String> parseHeaders(HttpURLConnection conn)
46 46
   {
47 47
     Map<String, String> headers = new HashMap<String, String>();
48  
-    for (String key : conn.getHeaderFields().keySet())
  48
+    for (Map.Entry<String, List<String>> entry : conn.getHeaderFields().entrySet())
49 49
     {
50  
-      headers.put(key, conn.getHeaderFields().get(key).get(0));
  50
+	  if(entry.getValue() != null) {
  51
+        headers.put(entry.getKey(), entry.getValue().get(0));
  52
+	  }
51 53
     }
52 54
     return headers;
53 55
   }
@@ -110,4 +112,4 @@ public String getHeader(String name)
110 112
     return headers.get(name);
111 113
   }
112 114
 
113  
-}
  115
+}
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.