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

Daan Hoogland daan.hoogland at luminis.nl
Mon Jun 20 16:53:00 PDT 2005


moving thing to disableIO seem reasonable, but you don't know how
TCPSession is being used so better set an explicit null.

Dann Martens wrote:

> 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