Jetty

NullPointerException when closing connection

Details

  • Type: Bug Bug
  • Status: Resolved Resolved
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 6.1.2rc1
  • Fix Version/s: 6.1.4rc0
  • Component/s: NIO
  • Labels:
    None
  • Environment:
    JDK 1.6.0, Solaris 9
  • Number of attachments :
    0

Description

I am getting an intermittent NPE when I try to close the output stream for a response. The channel is non-blocking and my code is running outside of the handler after a continuation was created. I previously wrote the header for an HTTP 1.0 style streaming response (i.e. no content length, non-chunked) on the connection and now I am trying to close the connection at the end of the document.

The exception I'm getting is:

java.lang.NullPointerException
        at org.mortbay.jetty.HttpFields$2.hasNext(HttpFields.java:267)
        at org.mortbay.jetty.HttpGenerator.completeHeader(HttpGenerator.java:379)
        at org.mortbay.jetty.HttpConnection.commitResponse(HttpConnection.java:529)
        at org.mortbay.jetty.HttpConnection$Output.close(HttpConnection.java:837)
        at com.electriccloud.accelerator.service.BuildNotifier.cleanup(BuildNotifier.java:77)

I have tried to trace the problem in the debugger, and it appears the NPE happens because destroy() has already been called on the connection so the attempt to commit a response is working on a partially destroyed object. As far as I can tell, destroy() is being called from the selector thread. My speculation is that the failure is caused by a race between the selector thread detecting a disconnect from the remote side and my thread deciding to close the channel.

I'm having a hard time narrowing down the problem to a small test case, but let me know if you want me to inject any logging because the problem is reproducible within 10-20 minutes.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Greg Wilkins added a comment -

Well this should be an ignorable exception - but it would be best if it was not a NPE.
Normally one gets and IOException in this situation.

I will look to see of destroy can be delayed until dispatched threads have returned.

Show
Greg Wilkins added a comment - Well this should be an ignorable exception - but it would be best if it was not a NPE. Normally one gets and IOException in this situation. I will look to see of destroy can be delayed until dispatched threads have returned.
Hide
Permalink
Greg Wilkins added a comment -

This could be a little tricky to prevent and I want to move carefully so as to not cause other problems and to slow performance.

How frequently does it occur for you - ie is this a very high priority?

An intermediate step may be to not pro actively destroy the fields and to allow normal garbage collection
to destroy them once handling has finished.

Show
Greg Wilkins added a comment - This could be a little tricky to prevent and I want to move carefully so as to not cause other problems and to slow performance. How frequently does it occur for you - ie is this a very high priority? An intermediate step may be to not pro actively destroy the fields and to allow normal garbage collection to destroy them once handling has finished.
Hide
Permalink
Scott Stanton added a comment -

It's happening every couple of minutes under load. I've added a catch around the close() call in my code to suppress all exceptions. Not ideal, but at least it gets rid of the extraneous log messages and doesn't seem to have any negative impact so far.

Would it be reasonable to have the close() call check for an already destroyed object and just do nothing on the second call?

Show
Scott Stanton added a comment - It's happening every couple of minutes under load. I've added a catch around the close() call in my code to suppress all exceptions. Not ideal, but at least it gets rid of the extraneous log messages and doesn't seem to have any negative impact so far. Would it be reasonable to have the close() call check for an already destroyed object and just do nothing on the second call?
Hide
Permalink
Greg Wilkins added a comment -

I am adding a check in commitResponse that looks to see if the connection has been reset and throws an EofException if it has.
This should give you the currect behaviour in 99% of cases.

There is still a race where the reset can be called after the check and before/during the completeHeader, but hopefully that will be
small.

I will look at delaying the reset until all dispatched methods have completed, but that will need a bit of thought, planning and debugging

Show
Greg Wilkins added a comment - I am adding a check in commitResponse that looks to see if the connection has been reset and throws an EofException if it has. This should give you the currect behaviour in 99% of cases. There is still a race where the reset can be called after the check and before/during the completeHeader, but hopefully that will be small. I will look at delaying the reset until all dispatched methods have completed, but that will need a bit of thought, planning and debugging
Hide
Permalink
Greg Wilkins added a comment -

destroy is delayed

Show
Greg Wilkins added a comment - destroy is delayed

People

  • Assignee:
    Greg Wilkins
    Reporter:
    Scott Stanton
Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: