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

Java Bayeux Client can send a disconnect message without a clientId #777

Closed
wlisac opened this Issue May 22, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@wlisac

wlisac commented May 22, 2018

I've noticed that it's possible for the Java Bayeux Client to send a /meta/disconnect message without a clientId.

According to the protocol documentation, a /meta/disconnect message must include a clientId.

Steps to Reproduce

  • Ask the client to handshake and then immediately attempt to disconnect.
client.handshake();
client.disconnect();

Expected Result

  • I'd expect the handshake() call to make the client enter into the handshaking session state and send a meta/handshake message.

  • I'd expect the following immediate disconnect() call to make the client enter into either the disconnecting or terminating state and not send a /meta/disconnect message since the client doesn't yet have a clientId to include in the message.

Actual Result

  • The handshake() call makes the client enter into the handshaking session state and send a meta/handshake message.

  • The following immediate disconnect() call makes the client enter into the disconnecting state and send a /meta/disconnect message without a clientId.

Additional Information

Looking at the code, it's currently legal for the client to transition directly from handshaking to disconnecting state:

private boolean isUpdateableTo(State newState) {
    switch (this) {
        case DISCONNECTED:
            return newState == HANDSHAKING;
        case HANDSHAKING:
        case REHANDSHAKING:
            return EnumSet.of(REHANDSHAKING, HANDSHAKEN, DISCONNECTING, TERMINATING).contains(newState);
        case HANDSHAKEN:
            return EnumSet.of(CONNECTING, DISCONNECTING, TERMINATING).contains(newState);
        case CONNECTING:
        case CONNECTED:
        case UNCONNECTED:
            return EnumSet.of(REHANDSHAKING, CONNECTED, UNCONNECTED, DISCONNECTING, TERMINATING).contains(newState);
        case DISCONNECTING:
            return newState == TERMINATING;
        case TERMINATING:
            return newState == DISCONNECTED;
        default:
            throw new IllegalStateException();
    }
}

I wonder if transitioning from handshaking or rehandshaking directly to disconnecting should be an illegal transition. Instead, maybe the client should transition directly to terminating since the client can't have a clientId when handshaking and may not have a clientId when rehandshaking. (Side note – would it make sense to always clear the clientId before rehandshaking anyway)?

Alternatively, maybe the disconnecting state could skip sending the /meta/disconnect message if the client is coming from handshaking or rehandshaking states. This would likely require updating the documentation for the disconnecting state since it currently implies that a disconnect message will be sent in this state.

/**
* State assumed when the disconnect is being sent
*/
DISCONNECTING,
@sbordet

This comment has been minimized.

Member

sbordet commented May 22, 2018

What CometD version are you using ?

client.handshake();
client.disconnect();

That is considered bad API usage.

Calls to handshake() are asynchronous and are not guaranteed to even be started when the handshake() call returns.

The clientId is returned by the server, so any disconnect that happens before the server replies to the handshake won't have a clientId to send.

Not sending the disconnect message is IMO worse than sending one without the clientId, especially for connection-aware transports such as WebSocket.

I don't understand if you are reporting this because it actually causes you a problem, or just for sake of precision with respect to the specification.

Thanks!

@wlisac

This comment has been minimized.

wlisac commented May 22, 2018

hi @sbordet – thanks for the quick reply :)

I'm writing a basic (long-polling) Bayeux client for a different platform and have been studying how the cometd client handles some edge cases (in an effort to keep my client relatively consistent).

I hadn't considered the connection-aware transport since my client is currently long-polling only.

My example above is clearly contrived, but I think it's a valid use case for an application to want to stop the client (via disconnect()) before the client has finished a successful handshake / has a valid clientId.

I'm not sure what the expected / defined behavior of the Bayeux Server is if a client sends a meta/disconnect message without a clientId. Maybe you have some insight there? In the case of a long-polling transport, I wonder if the server may attempt to disconnect all sessions associated with the BAYEUX_BROWSER cookie? I'd have to experiment a bit to figure that out / it may depend on the server implementation.

Really appreciate the help!

Cheers,
Will

@wlisac

This comment has been minimized.

wlisac commented May 22, 2018

It looks like, by default, the server fails the disconnect message if it's missing a clientId.

Request

{
	"id": "1",
	"channel": "\/meta\/disconnect"
}

Response

[{
	"channel": "/meta/disconnect",
	"id": "1",
	"error": "402::Unknown client",
	"successful": false
}]
@sbordet

This comment has been minimized.

Member

sbordet commented May 22, 2018

CometD API were designed a while ago, and today's design may be different and more up-to-date with current technologies and patterns.

So if you're designing new APIs, maybe you want to make sure that handshake() returns something on which you can call disconnect(), so that there is less ambiguity on how to use the API.

Regarding the case at hand, I don't think the server should disconnect all sessions for a particular BAYEUX_BROWSER if it gets a disconnect without clientId.
The current behavior is harmless, in a way: the client will be disconnected, and the server will time out that session - not immediately but eventually that session will be gone.

We may indeed avoid to send the disconnect message, though - it will have the same effect (client disconnected and server timing out the session).

@wlisac

This comment has been minimized.

wlisac commented May 22, 2018

Makes sense. Thanks again for the help!

@sbordet sbordet closed this Jul 30, 2018

@sbordet sbordet added this to the 3.1.5 milestone Sep 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment