[SNMP4J] NullPointerException while resending a GET request
Frank Fock
fock at agentpp.com
Sat Apr 11 11:35:59 CEST 2009
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
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: Snmp_1_9_3d.patch
URL: <http://oosnmp.net/pipermail/snmp4j/attachments/20090411/dd142afa/attachment.ksh>
More information about the SNMP4J
mailing list