StreamProxy should not NPE when socket is closed without request

In testing I found the the StreamProxy class was throwing a NullPointerException when trying to parse a null through StringTokenizer. This happens when the client connects to the proxy and disconnects without ever sending a request.

I suspect this would never happen in the app, but should be checked so that other tests can do this action.

This patch includes a test case to check for the problem and a patch. It also includes some code cleanup (created a constant for the logging in StreamProxy to match the other classes).

[Michael Frederick, 2010-09-21 00:38:09]

Nice! A few comments.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StreamProxyTest.java (right):

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:33: private boolean
caughtNPE = false;
Instead of putting this on the class, make it a local or instance variable of
the handler.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:46:
UncaughtExceptionHandler oldHanlder =
Thread.getDefaultUncaughtExceptionHandler();
"oldHandler"

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:46:
UncaughtExceptionHandler oldHanlder =
Thread.getDefaultUncaughtExceptionHandler();
Wrap line at 80 cols.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:47:
Thread.setDefaultUncaughtExceptionHandler(new ThreadExceptionHandler());
Rather than use an inner class, I would prefer an inline class here.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:64: if (caughtNPE) {
Use assertFalse instead.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:65: fail("Proxy isn't
cheking for null content when reading from buffer.");
"checking"

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:90: fail( "Proxy is not
running on localhost. " + e.getClass().getSimpleName() + ":" + e.getMessage());
Wrap line at 80 cols.

[jeremy.wadsack, 2010-09-21 17:30:01]

Updated to reflect comments and new patch set pushed.

Note that I can't do an inline class because it needs to set a boolean.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StreamProxyTest.java (right):

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:33: private boolean
caughtNPE = false;
On 2010/09/21 00:38:09, Michael Frederick wrote:
> Instead of putting this on the class, make it a local or instance variable of
> the handler.

Done.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:46:
UncaughtExceptionHandler oldHanlder =
Thread.getDefaultUncaughtExceptionHandler();
On 2010/09/21 00:38:09, Michael Frederick wrote:
> "oldHandler"

Done.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:46:
UncaughtExceptionHandler oldHanlder =
Thread.getDefaultUncaughtExceptionHandler();
On 2010/09/21 00:38:09, Michael Frederick wrote:
> Wrap line at 80 cols.

Done.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:47:
Thread.setDefaultUncaughtExceptionHandler(new ThreadExceptionHandler());
On 2010/09/21 00:38:09, Michael Frederick wrote:
> Rather than use an inner class, I would prefer an inline class here.

I can't use an inline class (although it would keep the test cleaner) because it
needs to set the caughtNPE value. That can't be in an inline class because it's
of type UncaughtExceptionHandler. And it can't be in the containing class
because it would have to be final then.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:64: if (caughtNPE) {
On 2010/09/21 00:38:09, Michael Frederick wrote:
> Use assertFalse instead.

Done.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:65: fail("Proxy isn't
cheking for null content when reading from buffer.");
On 2010/09/21 00:38:09, Michael Frederick wrote:
> "checking"

Done.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:90: fail( "Proxy is not
running on localhost. " + e.getClass().getSimpleName() + ":" + e.getMessage());
On 2010/09/21 00:38:09, Michael Frederick wrote:
> Wrap line at 80 cols.

Done.

[Michael Frederick, 2010-09-21 17:36:35]

LGTM

One more question.

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
File Npr_Test/src/org/npr/android/news/StreamProxyTest.java (right):

http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
Npr_Test/src/org/npr/android/news/StreamProxyTest.java:47:
Thread.setDefaultUncaughtExceptionHandler(new ThreadExceptionHandler());
On 2010/09/21 17:30:01, jeremy.wadsack wrote:
> On 2010/09/21 00:38:09, Michael Frederick wrote:
> > Rather than use an inner class, I would prefer an inline class here.
> 
> I can't use an inline class (although it would keep the test cleaner) because
it
> needs to set the caughtNPE value. That can't be in an inline class because
it's
> of type UncaughtExceptionHandler. And it can't be in the containing class
> because it would have to be final then.

Can't you do:
Thread.setDefaultUncaughtExceptionHandler(new UncaughtExceptionHandler() {
   @Override
   public void uncaughtException(Thread thread, Throwable ex) { ... }
});

[jeremy.wadsack, 2010-09-21 18:42:30]

I don't think so. I can't add a public variable or method there, so to do
that the caughtNPE boolean either needs to be local and final (in which case
it can't be set) or it needs to be a class variable on the containing class.
It's one or the other and I think having the state within the class is
cleaner than having an anonymous class set the containing class's member
variable.

--
Jeremy Wadsack


On Tue, Sep 21, 2010 at 10:36 AM, <mfrederick@google.com> wrote:

> LGTM
>
> One more question.
>
>
>
>
>
http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
> File Npr_Test/src/org/npr/android/news/StreamProxyTest.java (right):
>
>
>
http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
> Npr_Test/src/org/npr/android/news/StreamProxyTest.java:47:
> Thread.setDefaultUncaughtExceptionHandler(new ThreadExceptionHandler());
> On 2010/09/21 17:30:01, jeremy.wadsack wrote:
>
>> On 2010/09/21 00:38:09, Michael Frederick wrote:
>> > Rather than use an inner class, I would prefer an inline class here.
>>
>
>  I can't use an inline class (although it would keep the test cleaner)
>>
> because it
>
>> needs to set the caughtNPE value. That can't be in an inline class
>>
> because it's
>
>> of type UncaughtExceptionHandler. And it can't be in the containing
>>
> class
>
>> because it would have to be final then.
>>
>
> Can't you do:
> Thread.setDefaultUncaughtExceptionHandler(new UncaughtExceptionHandler()
> {
>   @Override
>   public void uncaughtException(Thread thread, Throwable ex) { ... }
> });
>
>
> http://codereview.appspot.com/2255041/
>

[Michael Frederick, 2010-09-21 18:44:19]

Agreed. Thanks!

On Tue, Sep 21, 2010 at 2:42 PM, Jeremy Wadsack <jeremy.wadsack@gmail.com>wrote:

> I don't think so. I can't add a public variable or method there, so to do
> that the caughtNPE boolean either needs to be local and final (in which case
> it can't be set) or it needs to be a class variable on the containing class.
> It's one or the other and I think having the state within the class is
> cleaner than having an anonymous class set the containing class's member
> variable.
>
> --
> Jeremy Wadsack
>
>
>
> On Tue, Sep 21, 2010 at 10:36 AM, <mfrederick@google.com> wrote:
>
>> LGTM
>>
>> One more question.
>>
>>
>>
>>
>>
http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
>> File Npr_Test/src/org/npr/android/news/StreamProxyTest.java (right):
>>
>>
>>
http://codereview.appspot.com/2255041/diff/1/Npr_Test/src/org/npr/android/new...
>> Npr_Test/src/org/npr/android/news/StreamProxyTest.java:47:
>> Thread.setDefaultUncaughtExceptionHandler(new ThreadExceptionHandler());
>> On 2010/09/21 17:30:01, jeremy.wadsack wrote:
>>
>>> On 2010/09/21 00:38:09, Michael Frederick wrote:
>>> > Rather than use an inner class, I would prefer an inline class here.
>>>
>>
>>  I can't use an inline class (although it would keep the test cleaner)
>>>
>> because it
>>
>>> needs to set the caughtNPE value. That can't be in an inline class
>>>
>> because it's
>>
>>> of type UncaughtExceptionHandler. And it can't be in the containing
>>>
>> class
>>
>>> because it would have to be final then.
>>>
>>
>> Can't you do:
>> Thread.setDefaultUncaughtExceptionHandler(new UncaughtExceptionHandler()
>> {
>>   @Override
>>   public void uncaughtException(Thread thread, Throwable ex) { ... }
>> });
>>
>>
>> http://codereview.appspot.com/2255041/
>>
>
>