Bug 53606

Summary: NullPointerException in TcpPingInterceptor
Product: Tomcat 6 Reporter: F.Arnoud <frederic.arnoud>
Component: ClusterAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: unspecified   
Target Milestone: default   
Hardware: PC   
OS: All   

Description F.Arnoud 2012-07-26 16:15:05 UTC
start(int) method initializes failureDetector (resp. staticMembers) only if TcpFailureDetector (resp. StaticMembershipInterceptor) was found in channel interceptors stack.

Without TcpFailureDetector (resp. StaticMembershipInterceptor), futur calls to sendPing() will fail because failureDetector (resp. staticMembers) wasn't initialized at least to new WeakReference<StaticMembershipInterceptor>(null).

Fix:
v1) initializes weak references containers:
Replace:
    WeakReference<TcpFailureDetector> failureDetector = null;
    WeakReference<StaticMembershipInterceptor> staticMembers = null;
for:
    WeakReference<TcpFailureDetector> failureDetector = new WeakReference<TcpFailureDetector>();
    WeakReference<StaticMembershipInterceptor> staticMembers = new WeakReference<StaticMembershipInterceptor>();

v2) checks field before dereferencing it:
sendPing becomes:
    protected void sendPing() {
        if (failureDetector!=null && failureDetector.get()!=null) {
            //we have a reference to the failure detector
            //piggy back on that dude
            failureDetector.get().checkMembers(true);
        }else {
            if (staticOnly && staticMembers!=null && staticMembers.get()!=null) {
                sendPingMessage(staticMembers.get().getMembers());
            } else {
                sendPingMessage(getMembers());
            }
        }
    }



affect also tomcat 6
regards
fred arnoud
Comment 1 F.Arnoud 2012-07-26 16:21:11 UTC
Sorry:

Forget first solution (v1) we cannot set a weak reference.
Need to modify sendPing method to check null pointer.

regards
fred
Comment 2 F.Arnoud 2012-07-26 16:26:39 UTC
I used this solution:

    protected void sendPing() {
        TcpFailureDetector tcpFailureDetector = failureDetector!=null ? failureDetector.get() : null;
        if (tcpFailureDetector!=null) {
            //we have a reference to the failure detector
            //piggy back on that dude
            tcpFailureDetector.checkMembers(true);
        }else {
            StaticMembershipInterceptor staticMembershipInterceptor = staticOnly && staticMembers!=null ? staticMembers.get() : null;
            if (staticMembershipInterceptor!=null) {
                sendPingMessage(staticMembershipInterceptor.getMembers());
            } else {
                sendPingMessage(getMembers());
            }
        }
    }
Comment 3 Sebb 2012-07-26 18:00:00 UTC
(In reply to comment #2)
> I used this solution:
> 
>     protected void sendPing() {
>         TcpFailureDetector tcpFailureDetector = failureDetector!=null ?
> failureDetector.get() : null;
>         if (tcpFailureDetector!=null) {

That's nice; and better than v2 as it also protects against another possible NPE, i.e.:

failureDetector.get() can return null (it's a WeakReference, so get() can return null at any time).
Comment 4 F.Arnoud 2012-07-26 18:36:18 UTC
You're right, only one access to get() for WeakReference (and brother classes).
Comment 5 Mark Thomas 2012-07-29 21:52:02 UTC
Fixed in trunk and 7.0.x and will be included in 7.0.30 onwards. Thanks for the patch.
Comment 6 Mark Thomas 2012-07-29 21:54:46 UTC
Whoops. Re-open for Tomcat 6.
Comment 7 Mark Thomas 2012-08-27 21:35:31 UTC
Fixed in 6.0.x and will be included in 6.0.36 onwards.