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

Channel class swallows errors for failed RPC to ChannelService.join() #6

Open
vidalborromeo opened this issue Aug 29, 2016 · 3 comments

Comments

@vidalborromeo
Copy link

Hi there, thanks for the great library.
In the Channel.join() method I noticed that the onError() callback for the RPC is empty, i.e. it will hide errors on failed calls to the server:

    public void join() {
        channelService.join(channelName, new AsyncCallback<String>() {
            @Override
            public void onFailure(Throwable throwable) {
                // Exceptions disappear here
            }

            @Override
            public void onSuccess(String t) {
                token = t;
                join(token);
            }
        });
    }

This is probably a rare event, but potentially tricky to debug when it happens.
I can think of a few potential solutions. Here's the first two:

  1. Add onJoinError() to ChannelListener:
public interface ChannelListener extends RemoteService {

    void onJoinError(Throwable caught);

    void onMessage(String message);

    void onOpen();

    void onError(int code, String description);

    void onClose();
}
  1. Add something like FailedJoinCallback and join(FailedJoinCallback callback) to Channel:
public interface FailedJoinCallback {
    void onFailedJoin(Throwable caught);
}
    public void join(FailedJoinCallback callback) {
        channelService.join(channelName, new AsyncCallback<String>() {
            @Override
            public void onFailure(Throwable caught) {
                callback.onFailedJoin(caught);
            }

            @Override
            public void onSuccess(String t) {
                token = t;
                join(token);
            }
        });
@eirikb
Copy link
Owner

eirikb commented Aug 29, 2016

Hi

I guess this problem applies to send as well, and one might want to get a callback on onSuccess.
Would it be odd to accept new AsyncCallback<String> as an argument to both join and send?
(Or as optional argument, additional method).
I'm not sure about the whole token stuff, but for onSuccess the success could be delegated.

@vidalborromeo
Copy link
Author

I think that could work, especially as an optional argument in an additional method. A usage example for new users would help avoid confusion between the two versions. For the callback passed to join(AsyncCallback<String>) you might also want to explain in a comment/javadoc the String they're receiving in onSuccess(String).

AsyncCallback<Void> is yet another option, but maybe not as widely useful as AsyncCallback<String>.

@eirikb
Copy link
Owner

eirikb commented Aug 30, 2016

Using AsyncCallback<Void> could be a good idea, as end users might not need to know about the token. Then again there's no reason not to give it out.

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