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


#1

Hey @pspeed

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


#2

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.


#3

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…


#4

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.


#5

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.


#6

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.


#7

@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 ?


#8

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


#9

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

Which will eventually call:

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).


#10

Okay, this PR should fix the NPE


#11

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?


#12

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);
        }
    }

#13

Looks good to me.


#14

Done


#15

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


#16

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).


#17

Thanks. Merged and release notes updated.


#18

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


#19

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


#20

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

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.