Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oort data is not deserialized starting from 5.0.5 #1037

Closed
zaynetro opened this issue May 11, 2021 · 10 comments
Closed

Oort data is not deserialized starting from 5.0.5 #1037

zaynetro opened this issue May 11, 2021 · 10 comments

Comments

@zaynetro
Copy link
Contributor

zaynetro commented May 11, 2021

We are migrating from 5.0.2 to 5.0.7.

// Configure bayeux (some of the options we configure)
bayeuxServer.setOption( AbstractServerTransport.JSON_CONTEXT_OPTION, new CometdJsonContextServer() );
bayeuxServer.addExtension( new AcknowledgedMessagesExtension() );

// Configure oort
oort.setJSONContextClient( new CometdJsonContextClient() );

Where Cometd server and client context have the same implementation:

public class CometdJsonContextServer extends JettyJSONContextServer {
    public CometdJsonContextServer() {
        // Trying this with 5.0.7
        putConvertor( QueuedChat.class.getName(), new QueuedChatJsonConvertor() );
        // Used this in 5.0.2
        //getJSON().addConvertor( QueuedChat.class, new QueuedChatJsonConvertor() );
    }
}

and the actual convertor:

public static class QueuedChatJsonConvertor implements JSON.Convertor {
        @Override
        public void toJSON( Object obj, Output out ) {
            QueuedChat chat = (QueuedChat) obj;
            out.addClass( QueuedChat.class );
            out.add( "id", chat.id );
        }

        @Override
        @SuppressWarnings( "rawtypes" )
        public Object fromJSON( Map obj ) {
            return new QueuedChat( (String) obj.get( "id" ) );
        }
    }

Then we start an OortList<QueuedChat> and use it across nodes in the oort cluster. Everything is working well in 5.0.2 but with 5.0.7 we get a lot of:

java.lang.ClassCastException: java.util.HashMap cannot be cast to com.example.Chat$QueuedChat

Upon closer inspection our QueuedChatJsonConvertor.fromJSON is never called with 5.0.7 release. While it was called from JSON.parseObject before the update. The data that other node receives includes the "class" field with the correct class name but the converter is not called anyway.

The only change we are trying is cometd upgrade from 5.0.2 to 5.0.7 and jetty upgrade from 9.4.33.v20201020 to 9.4.40.v20210413. Plus the converter change but with or without the converter change the end result remains the same.

P.S I will try to prepare a test project tomorrow unless you won't suggest what might be wrong here before that.

UPD: Seems like exceptions happen only starting from 5.0.5 release.

@sbordet
Copy link
Member

sbordet commented May 11, 2021

@zaynetro I can't think of a precise change that could cause this issue.

There have been changes to the JSON parser/generator but we do have tests that prove that serialization in Oort works fine:
https://github.com/cometd/cometd/blob/5.0.7/cometd-java/cometd-java-oort/src/test/java/org/cometd/oort/OortStartupTest.java#L208

If you can reproduce the issue will be great.

Bear in mind that you have to setup the custom converter in 3 places: in the server (for Oort), in Oort (for OortComet, for inter-oort communication), and in the remote clients.

@zaynetro
Copy link
Contributor Author

OK after some further inspection I was able to repro one issue: Object is not deserialized in a custom OortService when received from another node. Test repo: https://github.com/zaynetro/cometd-json-issue

In cometd 5.0.4 we get

I|2021-05-12 15:29:11,217|[WebSocketClient@289710123-38]|[MyOortService.java:53] Received item QueueItem [id=localhost:8081 monitored the queue.]

but with 5.0.5 the object is not deserialized

W|2021-05-12 15:22:05,846|[WebSocketClient@704948997-38]|[MyOortService.java:56] Cannot process a message {item={id=localhost:8081 monitored the queue., class=com.example.QueueService$QueueItem}}: java.util.HashMap cannot be cast to com.example.QueueService$QueueItem 

The OortList seems to be working in this example but with 5.0.5 there is a warning log entry that is not present with 5.0.4:

[WARNING] No Class for 'com.example.QueueService$QueueItem'

We configure custom JSON context in two places:

bayeuxServer.setOption( AbstractServerTransport.JSON_CONTEXT_OPTION, new CometdJsonContextServer() );
oort.setJSONContextClient( new CometdJsonContextClient() );

What would be a third place to set the converter up for OortService to work?

@zaynetro
Copy link
Contributor Author

zaynetro commented May 12, 2021

Actually, I can now see the same error with OortList in 5.0.5 when pushing more items to the queue: zaynetro/cometd-json-issue@7990f3f

E|2021-05-12 16:14:22,799|[poolScheduler1]|[SpringConfig.java:26] Error in scheduled task 
java.lang.ClassCastException: java.util.HashMap cannot be cast to com.example.QueueService$QueueItem
	at com.example.QueueService.monitorQueue(QueueService.java:44)

@zaynetro zaynetro changed the title Oort data is not deserialized in 5.0.7 Oort data is not deserialized starting from 5.0.5 May 14, 2021
@sbordet
Copy link
Member

sbordet commented May 17, 2021

@zaynetro thanks for the reproducer!

The issue is due to jetty/jetty.project#6287.

The workaround is to, for now, disable the WebSocket transport among Oort nodes, and use the HTTP transport instead.

This can be done using Oort.setClientTransportFactories(List.of(new JettyHttpClientTransport.Factory()));.
See also #1024.

I tried your reproducer with the HTTP transport and it works fine.

@zaynetro
Copy link
Contributor Author

Thanks for the deep investigation! It is definitely way beyond my level of understanding how things work.

@sbordet
Copy link
Member

sbordet commented May 25, 2021

@zaynetro another workaround is to use the x-class field rather than class, since x-class avoids class loading.

Along these lines:

class QueuedChatJsonConvertor implements Convertor {
    @Override
    public void toJSON(Object obj, Output out) {
        // Do not do this line.
        // out.addClass(obj.getClass()); // Adds the "class" field.

        // Do this line instead.
        out.add("x-class", obj.getClass()); // Adds the "x-class" field.

        ...
    }
}

@sbordet
Copy link
Member

sbordet commented May 25, 2021

@zaynetro we have a fix in mainline jetty-9.4.x.

Would you be able to build Jetty 9.4.42-SNAPSHOT, use that as a dependency on CometD, build CometD 5.0.8-SNAPSHOT, and try out if the fix works for you?

I know it's a long shot, but would be great if you can -- no pressure though.

@zaynetro
Copy link
Contributor Author

Thanks for the tips! Unfortunately, I don't have much time to test this at the moment.

sbordet added a commit that referenced this issue Jun 4, 2021
Improved WebAppTest to test custom JSON serialization.
Issue is fixed by upgrading to Jetty 9.4.42.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet
Copy link
Member

sbordet commented Jun 4, 2021

The issue is fixed with the upgrade to Jetty 9.4.42.

@sbordet sbordet closed this as completed Jun 4, 2021
sbordet added a commit that referenced this issue Jun 4, 2021
Improved WebAppTest to test to be similar to CometD 6.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
sbordet added a commit that referenced this issue Jun 7, 2021
Fixed WebAppTest to work with Java 8, where few classes come from the JDK.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@zaynetro
Copy link
Contributor Author

Seems to be working fine now. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants