[Solved] Zay-ES (net) filter error when no entities has been added with given component type

asser_fahrenholz · December 5, 2018, 9:55am

Not sure if this is my own fault, but just highlighting here (there’s an easy workaround). I got an error when client is creating a new FieldFilter, with a component that no entities has yet.

10:22:27,086 ERROR [KernelAdapter] Unhandled error, endpoint:NioEndpoint[1, java.nio.channels.SocketChannel[connected local=/127.0.0.1:4273 remote=/127.0.0.1:51552]], context:StringIdMessage[id=null, string=legacy]
java.lang.RuntimeException: Error executing:public void com.simsilica.es.server.HostedEntityData.getStringInfo(com.jme3.network.HostedConnection,com.simsilica.es.net.StringIdMessage)
at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:238) ~[zay-es-net-1.3.1.jar:?]
at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:59) ~[zay-es-net-1.3.1.jar:?]
at com.jme3.network.base.MessageListenerRegistry.messageReceived(MessageListenerRegistry.java:81) ~[jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.DefaultServer.dispatch(DefaultServer.java:341) ~[jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.DefaultServer$Redispatch.messageReceived(DefaultServer.java:674) ~[jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.DefaultServer$Redispatch.messageReceived(DefaultServer.java:669) ~[jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.KernelAdapter.dispatch(KernelAdapter.java:187) [jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.KernelAdapter.createAndDispatch(KernelAdapter.java:241) [jme3-networking-3.2.1-stable.jar:3.2-stable]
at com.jme3.network.base.KernelAdapter.run(KernelAdapter.java:284) [jme3-networking-3.2.1-stable.jar:3.2-stable]
Caused by: java.lang.NullPointerException
at com.simsilica.es.base.MemStringIndex.getStringId(MemStringIndex.java:90) ~[zay-es-1.3.0.jar:?]
at com.simsilica.es.server.HostedEntityData.getStringInfo(HostedEntityData.java:330) ~[zay-es-net-1.3.1.jar:?]
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_161]
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_161]
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_161]
at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_161]
at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:231) ~[zay-es-net-1.3.1.jar:?]
… 8 more

Example code that causes the client to halt (client application stops, server continues to run):

legacyFilter = FieldFilter.create(TileType.class, "type", ed.getStrings().getStringId(TileTypes.LEGACY, false));

Am I doing something wrong there? The easy work around is to create dummy entities with the given type (before client tries to create the filter), so the component type is added to the string index. But should it not be possible to create filters for types that no entities has yet ?

if I catch the error, I’ll have to create the filter later on - with a null-check then create-catch, which seems messy ? or is there another way ?

Kind regards,
A

pspeed · December 5, 2018, 10:19am

It has nothing to do with filtering or anything like that.

If you call this on its own:
ed.getStrings().getStringId(TileTypes.LEGACY, false)
…you will get the same error.

TileTypes.LEGACY has not been added to the string index yet when this is called and so there is an internal NPE.

asser_fahrenholz · December 5, 2018, 2:01pm

Yes.

I guess what I’m asking is what is a good practice for declaring and instancing the fieldfilters - the client can’t really know if the entities are there or not…

pspeed · December 5, 2018, 2:54pm

But it doesn’t really have anything to do with the filters… it’s the IDs.

Server starts up.
Something sets up those IDs.

…much later…
Client connects… IDs are already there now.

Ali_RS · December 5, 2018, 3:05pm

What I am confused about is NPE is thrown from MemStringIndex which is at server side which should actually crash down server, Yes ?

Shouldn’t we handle this like you did for RemoteStringIndex and SqlStringIndex so that it should return -1 instead of null.

https://github.com/jMonkeyEngine-Contributions/zay-es/commit/270310702e7bf6660d07f4a890b3c1e2fd23a25d

github.com

jMonkeyEngine-Contributions/zay-es/blob/09f6a8f702a5f08d5689c7b220741ef2a5affb25/extensions/Zay-ES-Net/src/main/java/com/simsilica/es/client/RemoteStringIndex.java#L104


            return result;
        }
        
        // Else ask remote
        result = remote.getStringId(s);
        if( result != null ) {
            idIndex.put(s, result);
            stringIndex.put(result, s);
            return result;
        }
        return -1;
    } finally {
        indexLock.writeLock().unlock();
    }               
}
@Override
public String getString( int id ) {
    indexLock.readLock().lock();
    try {

github.com

jMonkeyEngine-Contributions/zay-es/blob/09f6a8f702a5f08d5689c7b220741ef2a5affb25/src/main/java/com/simsilica/es/sql/SqlStringIndex.java#L128


                idToString.put(i, s);
                return i;           
            } catch( SQLException e ) {
                throw new RuntimeException("Error getting string ID for:" + s, e);
            }
        }
    }
    
    // If we weren't adding above then it might still be negative
    if( i < 0 ) {
        return -1;
    }
                                    
    stringToId.put(s, i);
    idToString.put(i, s);
 
    return i;           
}
@Override
public String getString( int id ) {

pspeed · December 5, 2018, 3:19pm

I mean, it could… if the filter is created early then it will just fail. If it’s cached then it will fail all the time forever.

It’s very strange for the server not to set up these IDs ahead of time.

So, yes, consistency-wise the code should be changed to return -1 in this case. Then he will just have a different type of error.

Ali_RS · December 5, 2018, 3:31pm

@pspeed can you please clarify this point.
Client is always asking for string ids through the RemoteStringIndex yes ?
And RemoteStringIndex is already considering the null value and returns -1. So It should not crash client but the server, yes ?

github.com

jMonkeyEngine-Contributions/zay-es/blob/09f6a8f702a5f08d5689c7b220741ef2a5affb25/extensions/Zay-ES-Net/src/main/java/com/simsilica/es/client/RemoteStringIndex.java#L104


      
                      return result;
                  }
                  
                  // Else ask remote
                  result = remote.getStringId(s);
                  if( result != null ) {
                      idIndex.put(s, result);
                      stringIndex.put(result, s);
                      return result;
                  }
                  return -1;
              } finally {
                  indexLock.writeLock().unlock();
              }               
          }
          
          @Override
          public String getString( int id ) {
          
              indexLock.readLock().lock();
              try {

asser_fahrenholz · December 5, 2018, 6:23pm

The id’s are only set up if entities are created with those components right? Is it strange to not create all entities on server start? Not sure I follow you here… I must misunderstand something

pspeed · December 5, 2018, 9:16pm

Yes, but:
result = remote.getStringId(s);
…on RemoteEntityData will send a message on through to the host.

Which will eventually call:

github.com

jMonkeyEngine-Contributions/zay-es/blob/09f6a8f702a5f08d5689c7b220741ef2a5affb25/extensions/Zay-ES-Net/src/main/java/com/simsilica/es/server/HostedEntityData.java#L324


      
              }
              
              // Releasing an entity set is (currently) a safe operation
              // to perform even if the set is in use at the time.  The client
              // already has to deal with the race condition of continuing to
              // get updates for a (from their perspective) released set anyway.        
              EntitySet set = activeSets.remove(msg.getSetId());
              set.release();            
          }
          
          public void getStringInfo( HostedConnection source, StringIdMessage msg ) {
              if( msg.getId() != null ) {
                  source.send(new StringIdMessage(msg.getRequestId(), 
                                                  ed.getStrings().getString(msg.getId())));   
              } else if( msg.getString() != null ) {
                  source.send(new StringIdMessage(msg.getRequestId(), 
                                                  ed.getStrings().getStringId(msg.getString(), false)));   
              } else {
                  throw new RuntimeException("Bad StringIdMessage:" + msg);
              }
          }

And that’s just calling:
ed.getStrings().getStringId(msg.getString(), false)

…which will never return null but throw an NPE if the string doesn’t exist in the index yet.

But but but… you created that CONSTANT on startup, didn’t you? I mean, Java is creating TileTypes.LEGACY on start-up. What a waste? No. That’s normal practice.

So if you expect to be able to use that constant on the clients as a string ID then you should prepopulate them. Somewhere in the server startup code:
ed.getStrings().getStringId(TileTypes.LEGACY, true)

You could even cache them if you like. Or just use ints instead of strings in the first place… they’re constants after all and there is no reason other than “I like to see them in the logs” to use strings, really.

If Ali_RS adds the fix to no longer throw an NPE (which is a good fix) then what will happen instead is that you will get a filter that will never match anything. You’ll pass it to your EntitySet or Entitycontainer or whatever and never get any items in it and wonder why.

Bottom line: what’s weird, super-duper-weird, is to EVER call getStringId(someString, false) (note: the false) unless you are already sure that the string has been created (or you are checking the result to see if it has already been created).

Ali_RS · December 6, 2018, 4:58am

Okay, this PR should fix the NPE
https://github.com/jMonkeyEngine-Contributions/zay-es/pull/19

pspeed · December 6, 2018, 5:09am

As a beggar, I hate to be a chooser… but the code I referenced in getStringIfno() should also be modified to return a null if it gets a -1… else the client will cache that -1 and never again check for a new value.

Can you add that too?

Ali_RS · December 6, 2018, 5:53am

Are you ok with this change ?

public void getStringInfo( HostedConnection source, StringIdMessage msg ) {
        if( msg.getId() != null ) {
            source.send(new StringIdMessage(msg.getRequestId(), 
                                            ed.getStrings().getString(msg.getId())));   
        } else if( msg.getString() != null ) {
            int stringId = ed.getStrings().getStringId(msg.getString(), false);
            source.send(new StringIdMessage(msg.getRequestId(), 
                                            stringId != -1 ? stringId : null));   
        } else {
            throw new RuntimeException("Bad StringIdMessage:" + msg);
        }
    }

pspeed · December 6, 2018, 5:56am

Looks good to me.

Ali_RS · December 6, 2018, 6:11am

Done

https://github.com/jMonkeyEngine-Contributions/zay-es/pull/19

asser_fahrenholz · December 6, 2018, 7:09am

Ah - it dawns on me now, to use a server-side cache-function, without adding the entities.

I get it now - I thought the filter would work regardless if the strings had been created at decleration point in time.

This clarifies it for me

pspeed · December 6, 2018, 7:14am

And note, for something where the values are all constants anyway, it’s kind of a waste to use Strings-converted-to-IDs when ID constants would have worked just as well for everything but log messages (which could be handled in other ways).

pspeed · December 6, 2018, 7:21am

Thanks. Merged and release notes updated.

asser_fahrenholz · December 6, 2018, 7:27am

Is it wasteful because of the network traffic size etc. ?

pspeed · December 6, 2018, 8:33am

Yeah… so not very wasteful… but still a lot more work than a straight int constant.

Ali_RS · February 17, 2019, 3:49pm

Sorry for bumping the thread.
Just updated to zay-es-net 1.4.0 and seems like something is broken with my pr

I am getting

java.lang.RuntimeException: Error executing:public void com.simsilica.es.server.HostedEntityData.getStringInfo(com.jme3.network.HostedConnection,com.simsilica.es.net.StringIdMessage)
	at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:238) ~[zay-es-net-1.4.0.jar:?]
	at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:59) ~[zay-es-net-1.4.0.jar:?]
	at com.jme3.network.base.MessageListenerRegistry.messageReceived(MessageListenerRegistry.java:81) ~[jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.DefaultServer.dispatch(DefaultServer.java:341) ~[jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.DefaultServer$Redispatch.messageReceived(DefaultServer.java:674) ~[jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.DefaultServer$Redispatch.messageReceived(DefaultServer.java:669) ~[jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.KernelAdapter.dispatch(KernelAdapter.java:187) [jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.KernelAdapter.createAndDispatch(KernelAdapter.java:241) [jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
	at com.jme3.network.base.KernelAdapter.run(KernelAdapter.java:284) [jme3-networking-3.3.0-SNAPSHOT.jar:3.3-6611]
Caused by: java.lang.NullPointerException
	at com.simsilica.es.server.HostedEntityData.getStringInfo(HostedEntityData.java:345) ~[zay-es-net-1.4.0.jar:?]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:?]
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:?]
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
	at java.base/java.lang.reflect.Method.invoke(Method.java:564) ~[?:?]
	at com.simsilica.es.net.AbstractMessageDelegator.messageReceived(AbstractMessageDelegator.java:231) ~[zay-es-net-1.4.0.jar:?]
	... 8 more

@pspeed maybe we should do it as you said earlier ?

For my own future reference: on the one hand it occurs to me that it would also have been correct to return the -1 to the client and let it detect that it shouldn’t cache the -1 like it doesn’t cache null (ie: treat null and -1 the same). And defensively, that’s probably a good idea.