[SNMP4J] NullPointerException while resending a GET request
George Tsaloukidis
gtsal at intracom.gr
Thu Apr 16 09:22:20 CEST 2009
Hi Frank,
Just contacting you to inform you that I have tested the new code
and all went well.
Best regards,
George Tsaloukidis
----- Original Message -----
From: "Frank Fock" <fock at agentpp.com>
To: "George Tsaloukidis" <gtsal at intracom.gr>
Cc: <snmp4j at agentpp.org>
Sent: Saturday, April 11, 2009 12:35 PM
Subject: Re: [SNMP4J] NullPointerException while resending a GET request
> Hi George,
>
> Thank you for your excellent analysis!
> Please find my solution attached as a
> patch. Basically I needed to change
> PendingRequest subclass only.
>
> Best regards,
> Frank
>
> George Tsaloukidis wrote:
>> Dear Frank,
>>
>>
>>
>> I have tested the solution with the usage of pendingRetry flag,
>>
>> but the problem still remains.
>>
>>
>>
>> I am afraid the pendingRetry flag is raised too late. As I wrote in the
>> first mail,
>>
>> target object can be null in pduHandleAssigned() due to two different
>> reasons:
>>
>>
>>
>> a) PendingRequest was constructed from the original
>> PendingRequest
>>
>> that had been canceled before the creation of the copy.
>>
>> That means the thread of TransportMapping informed for the receive of the
>>
>> response after the valuation of "retry" flag in PendingRequest.run()
>> method and
>>
>> main thread managed to call the cancel method on original PendingRequest
>>
>> before the creation of the copy. So the pendingRetry should become true
>>
>> when the retry flag is evaluated and not after.
>>
>>
>>
>> synchronized (pendingRequests) {
>>
>> retry = (!finished) && (retryCount > 0) && (!responseReceived);
>>
>> if(retry)
>>
>> {
>>
>> this.pendingRetry = true;
>>
>> }
>>
>> }
>>
>>
>>
>> By placing the assignment of flag inside the synchronized block, we are
>> sure that
>>
>> the main thread is waiting for the notify call (so cancel cannot be
>> called).
>>
>>
>>
>> b) cancel() method of the copy PendingRequest was called.
>>
>> This could only happen if main thread found that into the
>> pendingRequests list.
>>
>> This means TimerThread managed to execute "pendingRequests.put(handle,
>> this);"
>>
>> in pduHandleAssigned() and main thread removed it from the
>> pendingRequests list
>>
>> and called the cancel() method before the access of target from
>> TimerThread.
>>
>>
>>
>> We can use the pendingRetry flag to protect us in this case too.
>>
>> The flag has to be raised before the insertion of the copy into the
>> pendingRequests list
>>
>> and it can be turned again to false after the calculation of the "delay"
>> local variable.
>>
>>
>>
>> I am looking forward for your comments,
>>
>>
>> George Tsaloukidis.
>>
>>
>>
>> ----- Original Message ----- From: "Frank Fock" <fock at agentpp.com>
>> To: "George Tsaloukidis" <gtsal at intracom.gr>
>> Cc: <snmp4j at agentpp.org>
>> Sent: Monday, April 06, 2009 1:08 AM
>> Subject: Re: [SNMP4J] NullPointerException while resending a GET request
>>
>>
>>> Hi George,
>>>
>>> Synchronizing the PendingRequest.cancel() method causes deadlock
>>> problems. The source problem is the early object release
>>> in that method. The retry creation phase can be protected by
>>> a simple flag that is true (on) while the next retry is being
>>> prepared and not yet sent.
>>>
>>> Please replace the PendingRequest class in Snmp.java with the
>>> code below (or the next nightly build) and let me know if
>>> the NPE goes away:
>>>
>>> Best regards,
>>> Frank
>>>
>>>
>>> class PendingRequest extends TimerTask implements PduHandleCallback {
>>>
>>> private PduHandle key;
>>> protected int retryCount;
>>> protected ResponseListener listener;
>>> protected Object userObject;
>>>
>>> protected PDU pdu;
>>> protected Target target;
>>> protected TransportMapping transport;
>>>
>>> private int requestStatus = 0;
>>> // Maximum request status - allows to receive up to two reports
>>> and then
>>> // send the original request again. A value of 0 is used for
>>> discovery.
>>> private int maxRequestStatus = 2;
>>>
>>> private volatile boolean finished = false;
>>> private volatile boolean responseReceived = false;
>>>
>>> private volatile boolean pendingRetry = false;
>>>
>>>
>>> public PendingRequest(ResponseListener listener,
>>> Object userObject,
>>> PDU pdu,
>>> Target target,
>>> TransportMapping transport) {
>>> this.userObject = userObject;
>>> this.listener = listener;
>>> this.retryCount = target.getRetries();
>>> this.pdu = pdu;
>>> this.target = (Target) target.clone();
>>> this.transport = transport;
>>> }
>>>
>>> private PendingRequest(PendingRequest other) {
>>> other.pendingRetry = true;
>>> this.userObject = other.userObject;
>>> this.listener = other.listener;
>>> this.retryCount = other.retryCount - 1;
>>> this.pdu = other.pdu;
>>> this.target = other.target;
>>> this.requestStatus = other.requestStatus;
>>> this.responseReceived = other.responseReceived;
>>> this.transport = other.transport;
>>> }
>>>
>>> protected void registerRequest(PduHandle handle) {
>>>
>>> }
>>>
>>> public void responseReceived() {
>>> this.responseReceived = true;
>>> }
>>>
>>> public synchronized void pduHandleAssigned(PduHandle handle,
>>> Object pdu) {
>>> if (key == null) {
>>> key = handle;
>>> pendingRequests.put(handle, this);
>>> registerRequest(handle);
>>> if (logger.isDebugEnabled()) {
>>> logger.debug("Running pending "+
>>> ((listener instanceof SyncResponseListener) ?
>>> "sync" : "async")+
>>> " request with handle " + handle+
>>> " and retry count left "+retryCount);
>>> }
>>> long delay = timeoutModel.getRetryTimeout(target.getRetries() -
>>> retryCount,
>>> target.getRetries(),
>>> target.getTimeout());
>>> if ((!finished) && (!responseReceived)) {
>>> timer.schedule(this, delay);
>>> }
>>> }
>>> }
>>>
>>> /**
>>> * Process retries of a pending request.
>>> */
>>> public synchronized void run() {
>>> PduHandle m_key = key;
>>> PDU m_pdu = pdu;
>>> Target m_target = target;
>>> TransportMapping m_transport = transport;
>>> ResponseListener m_listener = listener;
>>> Object m_userObject = userObject;
>>>
>>> if ((m_key == null) || (m_pdu == null) || (m_target == null) ||
>>> (m_listener == null)) {
>>> if (logger.isDebugEnabled()) {
>>> logger.debug("PendingRequest canceled key="+m_key+",
>>> pdu="+m_pdu+
>>> ", target="+m_target+", transport="+m_transport+",
>>> listener="+
>>> m_listener);
>>> }
>>> return;
>>> }
>>>
>>> try {
>>> boolean retry;
>>> synchronized (pendingRequests) {
>>> retry = (!finished) && (retryCount > 0) &&
>>> (!responseReceived);
>>> }
>>> if (retry) {
>>> try {
>>> PendingRequest nextRetry = new PendingRequest(this);
>>> sendMessage(m_pdu, m_target, m_transport, nextRetry);
>>> this.pendingRetry = false;
>>> }
>>> catch (IOException ex) {
>>> finished = true;
>>> logger.error("Failed to send SNMP message to " + m_target +
>>> ": " +
>>> ex.getMessage());
>>>
>>> messageDispatcher.releaseStateReference(m_target.getVersion(),
>>> m_key);
>>> listener.onResponse(new ResponseEvent(Snmp.this, null,
>>> m_pdu, null,
>>> m_userObject,
>>> ex));
>>> }
>>> }
>>> else if (!finished) {
>>> finished = true;
>>> pendingRequests.remove(m_key);
>>>
>>> if (!responseReceived) {
>>> // request timed out
>>> if (logger.isDebugEnabled()) {
>>> logger.debug("Request timed out: " +
>>> m_key.getTransactionID());
>>> }
>>>
>>> messageDispatcher.releaseStateReference(m_target.getVersion(),
>>> m_key);
>>> listener.onResponse(new ResponseEvent(Snmp.this, null,
>>> m_pdu, null,
>>> m_userObject));
>>> }
>>> }
>>> else {
>>> // make sure pending request is removed even if response
>>> listener
>>> // failed to call Snmp.cancel
>>> pendingRequests.remove(m_key);
>>> }
>>> }
>>> catch (RuntimeException ex) {
>>> logger.error("Failed to process pending request " + m_key +
>>> " because " + ex.getMessage(), ex);
>>> throw ex;
>>> }
>>> catch (Error er) {
>>> logger.fatal("Failed to process pending request " + m_key +
>>> " because " + er.getMessage(), er);
>>> throw er;
>>> }
>>> }
>>>
>>> public boolean setFinished() {
>>> boolean currentState = finished;
>>> this.finished = true;
>>> return currentState;
>>> }
>>>
>>> public void setMaxRequestStatus(int maxRequestStatus) {
>>> this.maxRequestStatus = maxRequestStatus;
>>> }
>>>
>>> public int getMaxRequestStatus() {
>>> return maxRequestStatus;
>>> }
>>>
>>> public boolean isResponseReceived() {
>>> return responseReceived;
>>> }
>>>
>>> /**
>>> * Cancels the request and clears all internal fields by setting
>>> them
>>> * to <code>null</code>.
>>> * @return
>>> * <code>true</code> if cancellation was successful.
>>> */
>>> public boolean cancel(){
>>> boolean result = super.cancel();
>>>
>>> // free objects early
>>> if (!pendingRetry) {
>>> key = null;
>>> pdu = null;
>>> target = null;
>>> transport = null;
>>> listener = null;
>>> userObject = null;
>>> }
>>> return result;
>>> }
>>> }
>>>
>>>
>>> George Tsaloukidis wrote:
>>>> Dear Frank,
>>>>
>>>> I am using snmp4j version 1.9.3d for executing synchronous snmp GET
>>>> requests.
>>>> My application has many threads, but each thread uses its own
>>>> org.snmp4j.Snmp instance.
>>>> Sometimes I get the following exception:
>>>>
>>>> java.lang.NullPointerException
>>>> at org.snmp4j.Snmp$PendingRequest.pduHandleAssigned(Unknown
>>>> Source)
>>>> at org.snmp4j.MessageDispatcherImpl.sendPdu(Unknown Source)
>>>> at org.snmp4j.Snmp.sendMessage(Unknown Source)
>>>> at org.snmp4j.Snmp$PendingRequest.run(Unknown Source)
>>>> at java.util.TimerThread.mainLoop(Timer.java:512)
>>>> at java.util.TimerThread.run(Timer.java:462)
>>>> Exception in thread "Timer-4877612" java.lang.NullPointerException
>>>> at org.snmp4j.Snmp$PendingRequest.pduHandleAssigned(Unknown
>>>> Source)
>>>> at org.snmp4j.MessageDispatcherImpl.sendPdu(Unknown Source)
>>>> at org.snmp4j.Snmp.sendMessage(Unknown Source)
>>>> at org.snmp4j.Snmp$PendingRequest.run(Unknown Source)
>>>> at java.util.TimerThread.mainLoop(Timer.java:512)
>>>> at java.util.TimerThread.run(Timer.java:462)
>>>>
>>>> After rebuilding SNMP4J.jar with debug information I saw that the
>>>> problem was at line 1449 of Snmp.java file:
>>>>
>>>> long delay = timeoutModel.getRetryTimeout(target.getRetries() -
>>>> retryCount,
>>>> target.getRetries(),
>>>> target.getTimeout());
>>>>
>>>> That means the target object was null while trying to retransmit the
>>>> request.
>>>> That can happen only for two reasons: a) PendingRequest of the retry
>>>> action created
>>>> just after the cancellation of the very first PendingRequest (line
>>>> 1487) or,
>>>> the PendingRequest canceled while inside the pduHandleAssigned()
>>>> method.
>>>> The second case can appear only if the main thread gets that object
>>>> from the pendingRequests list (line 811).
>>>> That means the Timer thread had previously managed to execute the
>>>> 1440 line and suspended for a while.
>>>>
>>>> The problem is that the send() method (line 793) can call the
>>>> cancel() operation on a PendingRequest
>>>> at any time, without taking into consideration if the TimerThread is
>>>> executed.
>>>>
>>>> A solution can be to first lock the PendingRequest before calling
>>>> setFinished() and cancel() methods.
>>>> Line 811 should also go after the cancellation of the first request
>>>> or else if PendingRequest.run() has started,
>>>> the newly created PendingRequest is left into the pendingRequests
>>>> list for ever.
>>>> I have tested the above solution and I had no problem.
>>>>
>>>> And something else. At line 810 the wait() method is called.
>>>> Sun advices such calls to be wrapped into a while loop
>>>> (see
>>>> http://java.sun.com/docs/books/jls/third_edition/html/memory.html#17.8.1
>>>> and
>>>> http://java.sun.com/javase/6/docs/api/java/lang/Object.html#wait()
>>>> links)
>>>>
>>>> while(syncResponse.response == null)
>>>> {
>>>> syncResponse.wait();
>>>> }
>>>>
>>>> Best regards,
>>>>
>>>> George Tsaloukidis,
>>>> Senior Software Engineer
>>>> Systems Software Developement
>>>> Intracom Telecom S.A
>>>> _______________________________________________
>>>> SNMP4J mailing list
>>>> SNMP4J at agentpp.org
>>>> http://lists.agentpp.org/mailman/listinfo/snmp4j
>>>
>>> --
>>> AGENT++
>>> http://www.agentpp.com
>>> http://www.mibexplorer.com
>>> http://www.mibdesigner.com
>>>
>>
>
> --
> AGENT++
> http://www.agentpp.com
> http://www.snmp4j.com
> http://www.mibexplorer.com
> http://www.mibdesigner.com
>
--------------------------------------------------------------------------------
> Index: Snmp.java
> ===================================================================
> --- Snmp.java (revision 459)
> +++ Snmp.java (working copy)
> @@ -804,7 +804,9 @@
> new PendingRequest(syncResponse, target, pdu, target,
> transport);
> handle = sendMessage(pdu, target, transport, request);
> try {
> - syncResponse.wait();
> + while (syncResponse.getResponse() == null) {
> + syncResponse.wait();
> + }
> retryRequest = (PendingRequest) pendingRequests.remove(handle);
> if (logger.isDebugEnabled()) {
> logger.debug("Removed pending request with handle: "+handle);
> @@ -1415,7 +1417,6 @@
> }
>
> private PendingRequest(PendingRequest other) {
> - other.pendingRetry = true;
> this.userObject = other.userObject;
> this.listener = other.listener;
> this.retryCount = other.retryCount - 1;
> @@ -1427,7 +1428,7 @@
> }
>
> protected void registerRequest(PduHandle handle) {
> -
> + // overwritten by subclasses
> }
>
> public void responseReceived() {
> @@ -1437,22 +1438,31 @@
> public synchronized void pduHandleAssigned(PduHandle handle, Object
> pdu) {
> if (key == null) {
> key = handle;
> - pendingRequests.put(handle, this);
> - registerRequest(handle);
> - if (logger.isDebugEnabled()) {
> - logger.debug("Running pending "+
> - ((listener instanceof SyncResponseListener) ?
> - "sync" : "async")+
> - " request with handle " + handle+
> - " and retry count left "+retryCount);
> + // get pointer to target before adding request to pending list
> + // to make sure that this retry is not being cancelled before we
> + // got the target pointer.
> + Target t = target;
> + if (t != null) {
> + pendingRequests.put(handle, this);
> + registerRequest(handle);
> + if (logger.isDebugEnabled()) {
> + logger.debug("Running pending " +
> + ((listener instanceof SyncResponseListener) ?
> + "sync" : "async") +
> + " request with handle " + handle +
> + " and retry count left " + retryCount);
> + }
> + long delay =
> + timeoutModel.getRetryTimeout(t.getRetries() - retryCount,
> + t.getRetries(),
> + t.getTimeout());
> + if ((!finished) && (!responseReceived)) {
> + timer.schedule(this, delay);
> + }
> + else {
> + pendingRequests.remove(handle);
> + }
> }
> - long delay = timeoutModel.getRetryTimeout(target.getRetries() -
> - retryCount,
> - target.getRetries(),
> - target.getTimeout());
> - if ((!finished) && (!responseReceived)) {
> - timer.schedule(this, delay);
> - }
> }
> }
>
> @@ -1478,11 +1488,11 @@
> }
>
> try {
> - boolean retry;
> synchronized (pendingRequests) {
> - retry = (!finished) && (retryCount > 0) && (!responseReceived);
> + this.pendingRetry =
> + (!finished) && (retryCount > 0) && (!responseReceived);
> }
> - if (retry) {
> + if (this.pendingRetry) {
> try {
> PendingRequest nextRetry = new PendingRequest(this);
> sendMessage(m_pdu, m_target, m_transport, nextRetry);
>
More information about the SNMP4J
mailing list