adds WebSocketListener at WebSocketService

[zacscoding]

What does this PR do?

added WebSocketListener at WebSocketService

Where should the reviewer start?

WebSocketService, WebSocketServiceTest

Why is it needed?

I want to handle websocket events such as onClose event.
I think that it will be better if WebSocketClient have multiple listeners and WebSocketSerivce`s listener
is added.

[snazha-blkio]

Hi @zacscoding

It would make sense to have something like this at the lower level within WebSocketClient.
In there perhaps we could have a List<WebSocketListener> that is traversed and notified of events from within WebSocketClient.

Then modify WebSocketService to include a addWebSocketListener() method that will add the argument to the webSocketClient member.

[iikirilov]

You can see checkstyle errors by running ./gradlew check locally.

I want to handle websocket events such as onClose event.

I think a cleaner approach is to overload setWebSocketListener() to allow it to take functions as parameters which are invoked in the handlers. Something like:

    private void setWebSocketListener(Consumer<String> onMessage, Consumer<Exception> onError, Runnable onClose) {
        webSocketClient.setListener(new WebSocketListener() {
            @Override
            public void onMessage(String message) throws IOException {
                onWebSocketMessage(message);
                onMessage.accept(message);
            }

            @Override
            public void onError(Exception e) {
                log.error("Received error from a WebSocket connection", e);
                onError.accept(e);
            }

            @Override
            public void onClose() {
                onWebSocketClose();
                onClose.run();
            }
        });
    }

I think that it will be better if WebSocketClient have multiple listeners and WebSocketSerivce`s listener
is added.

AFAIK WebSocketClient can only have 1 listener.

To get this functionality we need to maintain a list (probably a map) of WebSocketClient in WebSocketService

[zacscoding]

how about this ?

    public void connect() throws ConnectException {
        connect(null, null, null);
    }

    public void connect(Consumer<String> onMessage, Consumer<Throwable> onError, Runnable onClose) throws ConnectException {
        try {
            connectToWebSocket();
            setWebSocketListener(onMessage, onError, onClose);
        } catch (InterruptedException e) {
            Thread.currentThread().interrupt();
            log.warn("Interrupted while connecting via WebSocket protocol");
        }
    }

    private void connectToWebSocket() throws InterruptedException, ConnectException {
        boolean connected = webSocketClient.connectBlocking();
        if (!connected) {
            throw new ConnectException("Failed to connect to WebSocket");
        }
    }

    private void setWebSocketListener(Consumer<String> onMessage, Consumer<Throwable> onError, Runnable onClose) {
        webSocketClient.setListener(new WebSocketListener() {
            @Override
            public void onMessage(String message) throws IOException {
                onWebSocketMessage(message);
                if (onMessage != null) {
                    onMessage.accept(message);
                }
            }

            @Override
            public void onError(Exception e) {
                log.error("Received error from a WebSocket connection", e);
                if (onError != null) {
                    onError.accept(e);
                }
            }

            @Override
            public void onClose() {
                onWebSocketClose();
                if (onClose != null) {
                    onClose.run();
                }
            }
        });
    }

I don`t know how to maintain multiple WebSocketClient in WebSocketService TT

[snazha-blkio]

Your proposed solution re: consumers would be less intrusive on the tests, so i am ok with it.
@iikirilov ?

[zacscoding]

added functional listeners at WebSocketService.
I hope to be applied this commit because i am using this code by define again :(

[snazha-blkio]

@zacscoding , use of Runnable can be confusing as it would imply the implementation is invoked by another thread. Could you instead use Consumer<Void> please?
@iikirilov , thoughts?

[zacscoding]

If use Consumer, then have to called like onClose.accept(null);
is ok ?

[iikirilov]

yes

[zacscoding]

changed to consumer :)

[snazha-blkio]

@zacscoding can you please resolve the travis-ci checks, as per Ivaylo's suggestion?

[zacscoding]

appreciate ur kindness 👍

[zacscoding]

@snazha-blkio I updated source with checkstyle. is any problem?

[snazha-blkio]

@snazha-blkio I updated source with checkstyle. is any problem?

Could you please add some unit tests for your changes? We can then look at getting this into 5.0

[zacscoding]

@snazha-blkio added some testing code !

[snazha-blkio]

Thanks @zacscoding , this will go out in 5.0

[olimsaidov]

@snazha-blkio is there any reason why this pull request cannot be released on 4.* versions? There is no breaking changes in public interface.

We really need this feature. Thank you.

[iikirilov]

@olimsaidov yes we can do this