eclipse-ee4j / grizzly

Grizzly
https://eclipse-ee4j.github.io/grizzly
Other
147 stars 68 forks source link

Fixed NPE in toString #2162

Closed dmatej closed 2 years ago

dmatej commented 2 years ago

Signed-off-by: David Matějček dmatej@seznam.cz

dmatej commented 2 years ago

Don't merge yet, neither previous nor this version works - it is a trap with generics :-/

kofemann commented 2 years ago

Instead of 'don't merge' warning you should change the PR type to 'Draft', then the merge will be disabled until you release it.

pzygielo commented 2 years ago

Don't merge yet, neither previous nor this version works - it is a trap with generics :-/

Perhaps UT would be helpful. For example of

package org.glassfish.grizzly.filterchain;

import org.junit.Test;

public class FilterChainContextTest {
  @Test
  public void testToStringWithNonNullMessage() {
    var ctx = new FilterChainContext();
    char [] msg = "Grizzly Test".toCharArray();
    ctx.setMessage(msg);

    var str = ctx.toString();

    assert "FilterChainContext [connection=null, closeable=null, operation=NONE, message=Grizzly Test, address=null]".equals(str) : str;
  }

  @Test
  public void testToStringWithNullMessage() {
    var ctx = new FilterChainContext();
    char [] msg = null;
    ctx.setMessage(msg);

    var str = ctx.toString();

    assert "FilterChainContext [connection=null, closeable=null, operation=NONE, message=null, address=null]".equals(str) : str;
  }

  @Test
  public void testToStringWithNullObjectMessage() {
    var ctx = new FilterChainContext();
    Object msg = null;
    ctx.setMessage(msg);

    var str = ctx.toString();

    assert "FilterChainContext [connection=null, closeable=null, operation=NONE, message=null, address=null]".equals(str) : str;
  }
}

two tests fail with NPEx on master, and don't on this PR. Could this trap be shown with the test?

dmatej commented 2 years ago

Instead of 'don't merge' warning you should change the PR type to 'Draft', then the merge will be disabled until you release it.

If I could I would do that, but there is no such option (Drafts disabled on this repo?)

The bug was visible in glassfish tests with full logging and also from Eclipse editor. Simply null typed as means "use the most concrete method compatible, which is with char[] argument, not Object." Quite interesting, but String.valueOf(char[]) doesn't like null.

dmatej commented 2 years ago

Original version vs. trivial test: image

dmatej commented 2 years ago

Note - grizzly-framework tests passed on my machine, seems like some conflict on ports. Also my test was executed after failing tests.

Error:  Tests run: 8, Failures: 0, Errors: 2, Skipped: 0, Time elapsed: 1.209 s <<< FAILURE! - in org.glassfish.grizzly.TCPNIOTransportTest
Error:  org.glassfish.grizzly.TCPNIOTransportTest.testClose  Time elapsed: 0.237 s
Error:  org.glassfish.grizzly.TCPNIOTransportTest.testThreadInterruptionDuringAcceptDoesNotMakeServerDeaf  Time elapsed: 0.554 s
Error:  org.glassfish.grizzly.TCPNIOTransportTest.testMultiBind  Time elapsed: 0.008 s  <<< ERROR!
java.net.BindException: Address already in use
    at org.glassfish.grizzly.TCPNIOTransportTest.testMultiBind(TCPNIOTransportTest.java:139)