saltyrtc / saltyrtc-client-java

SaltyRTC Java implementation.
Apache License 2.0
3 stars 4 forks source link

Is this client thread-safe? #113

Closed ovalseven8 closed 5 years ago

ovalseven8 commented 5 years ago

I get null pointer exceptions when I connect/disconnect simultaneously:

[ConnectThread] ERROR SaltyRTC.ISignaling - WebSocket callback error: java.lang.NullPointerException
java.lang.NullPointerException
    at org.saltyrtc.client.signaling.Signaling$1.onConnectError(Signaling.java:330)
    at com.neovisionaries.ws.client.ListenerManager.callOnConnectError(ListenerManager.java:206)
    at com.neovisionaries.ws.client.ConnectThread.handleError(ConnectThread.java:46)
    at com.neovisionaries.ws.client.ConnectThread.runMain(ConnectThread.java:36)
    at com.neovisionaries.ws.client.WebSocketThread.run(WebSocketThread.java:45)

It's this code: https://github.com/saltyrtc/saltyrtc-client-java/blob/87fd191daae0f2ff727de497f218609d90facda4/src/main/java/org/saltyrtc/client/signaling/Signaling.java#L330

Following test:

    @Test
    public void testConnectDisconnect() throws Exception {
        // Create peers
        final SSLContext sslContext = SSLContextHelper.getSSLContext();
        final SaltyRTC initiator = new SaltyRTCBuilder(this.cryptoProvider)
            .connectTo(Config.SALTYRTC_HOST, Config.SALTYRTC_PORT, sslContext)
            .withKeyStore(new KeyStore(this.cryptoProvider))
            .usingTasks(new Task[]{ new DummyTask() })
            .asInitiator();
        final SaltyRTC responder = new SaltyRTCBuilder(this.cryptoProvider)
            .connectTo(Config.SALTYRTC_HOST, Config.SALTYRTC_PORT, sslContext)
            .withKeyStore(new KeyStore(this.cryptoProvider))
            .usingTasks(new Task[]{ new DummyTask() })
            .initiatorInfo(initiator.getPublicPermanentKey(), initiator.getAuthToken())
            .asResponder();

        for (int i = 0; i <= 200; i++) {
            new Thread(new Runnable() {
                @Override
                public void run() {
                    try {
                        initiator.connect();
                        initiator.disconnect();
                    } catch (ConnectionException e) {
                        e.printStackTrace();
                    }
                }
            }).start();
        }
    }
lgrahl commented 5 years ago

The original plan of @dbrgn was to make it thread-safe but, after a brief discussion a while ago, we concluded that it isn't thread-safe.

My personal opinion is also that there's no benefit in making it thread-safe.

ovalseven8 commented 5 years ago

Thanks for the clarification.

However, it would probably make sense to document that code from saltyrtc-client-java has to be called from one and the same thread only.