[BEEPbuilders] [beepcore] Caught NPE on SessionEvent.toString() when receivingsession close event

Dann Martens dann at tomoton.com
Mon Jun 20 16:19:02 PDT 2005


Hi,

I agree, some of these calls 'seem' (cautious) unnecessary. Shouldn't 
the socket clean-up occur in the disableIO method itself ?

org.beepcore.beep.transport.tcp.TCPSession:208
--- SNIP ---
    // Overrides method in Session
    public void terminate(String reason)
    {
        super.terminate(reason);

        if (socket != null) {
            try {
                socket.close();
            } catch (IOException e) {
            }

            socket = null;
            running = false;
        }
    }
--- SNIP ---

could be rewritten as:
--- SNIP ---
// NO LONGER NECESSARY: OVERRIDE TO BE REMOVED
public void terminate(String reason) {

    // Clean-up superclass.
    super.terminate(reason);
}
protected void disableIO() {

    running = false;

    // Clean up this subclass' specfics.
    try { socket.close(); } catch (Exception ignore) {}
}
--- SNIP ---

'running=false;' was already executed as well by:

org.beepcore.beep.transport.tcp.TCPSession: 236: disableIO(..
    running = false;
org.beepcore.beep.SessionImpl:582: terminate(..
    this.disableIO();
org.beepcore.beep.transport.tcp.TCPSession: 211: terminate(..
    super.terminate(reason);

The socket is specific to the TCPSession implementation, it should not 
be accessible through a public terminate method directly.

I would not consider it necessary to set the socket to NULL, unless 
TCPSession objects are used in a FlyWeight pattern or similar, which 
does not appear to be the case.

Plus, if Huston still has fixes out there for the framing mechanism (see 
these classes), we might be doing work for nothing, here... if at least 
he intended to fix those issues...

Cya,
 Dann


Christian Möller wrote:

>Dann Martens schrieb:
>  
>
>>I'll start posting my patches ASAP, together with the rationale for each
>>change or fix.
>>    
>>
>
>OK, back to the initial problem: Seems that
>org.beepcore.beep.transport.tcp.TCPSession.processNextFrame(), line 464
>sets socket refernce to NULL prematurely:
>
>	} catch (IOException e) {
>		log.error(e);
>
>		socket = null; // <== HERE
>
>		terminate(e.getMessage());
>	}
>
>Next call to "terminate" method will invoke session listeners further
>down the call chain (via "super.terminate(reason)", line 213). On the
>other hand the "toString" method references the socket variable.
>Changing the calls according to this will banish the NPE:
>
>	} catch (IOException e) {
>		log.error(e);
>		terminate(e.getMessage());
>		socket = null;
>	}
>
>Maybe it would be better setting socket variable to NULL within a
>finally block? Maybe it's not worth the effort because within the body
>of "terminate" the socket will be closed and the reference discarded anyway.
>
>  
>
>>P.S. Does everybody have the O'Reilly BEEP book, for reference ?
>>    
>>
>Yes
>
>Christian
>
>  
>



More information about the BEEPbuilders mailing list