[ADDED] Subscription.Close() allows closing durable

[kozlovic]

Resolves #114
Will require nats-io/nats-streaming-server#182
to be implemented.

[coveralls]

Changes Unknown when pulling 77819a9 on add_subscription_close into * on master*.

[kozlovic]

@sfrooster If/when that is merged, I would need to vendor the protobuf into the server and implement the server side (fast since code is already done, just need to vendor it).
Do you think we should return an error on Close() regardless if this is a durable or not, when connecting to an older server? Overall, does this look like it would fit the bill for what you need?

[sfrooster]

@kozlovic This looks good to me. Erroring when the server doesn't support the protocol/operation avoids the implicit error, so that's good. Is there anyway for a client to know (on connect) what version the server is, or what protocol version or operations it supports? If not, that may be a good future item. Either way, this seems good to me.

[kozlovic]

@sfrooster Yes, the client sends its protocol version on connect and receives the server protocol version in the connect response. Are you asking to expose that?

The client API can then return an error when Close() is called with a server that does not have the correct protocol version. This is important when a durable calls Close() because otherwise, the server would process the close as an unsubscribe request.

The question was, should I simply always fail if connecting to an older server, regardless if it is a durable or not (although non-durable don't really care).

[kozlovic]

By the way, I first coded the protocol as a feature bitmask, with even a Features() connect option that could make the client behave a certain way, and would have done the same for the server, but the interest for that for me was for testing, but I am afraid that it could open door to bad behavior, so decided to fallback to simple incremental protocol version.

[sfrooster]

Ah, you were asking about failing on connect. No, I don't think you should do that because it makes a mismatch too painful. I thought you were asking about erroring if close is called with a client that doesn't support it, which I think does make sense. Exposing the protocol/server version to the client would be a nice way for the client to tailor the calls it makes for different servers.

[kozlovic]

@sfrooster No, there is a misunderstanding. I am not saying that client should error out on connect. A new client can connect to an older server without any difference in behavior than it was connecting to a new server expect for the Close() API. The question I had was: should sub.Close() returns an error when connected to an old server, regardless if sub is a durable or not. As you can see from the code, I return an error if connected to an older server and the sub is durable. If not durable I let go because the old server will behave the same than new server for a Close() with non-durable subs.

[sfrooster]

@kozlovic I think it seems reasonable enough as you have it.

[kozlovic]

Was experimenting with bitmask again and exposing the server's "features" as we discussed. Then I realized that client does not really connect to a server, so if server of different versions are stopped and restarted, client would get the server's features/proto only from the original connect response. Something to think about...

[coveralls]

Changes Unknown when pulling 7219ce2 on add_subscription_close into * on master*.

[kozlovic]

@sfrooster Here is a new take. It does not use protocol version, but adds a new SubscriptionCloseRequest protocol message that is sent to the server. Since the server has to listen for this protocol on a new subject, the effect is same as having a protocol version field. The client will be able to detect that it has connected to an old server since the subscription close request subject will not be set in the ConnectResponse of an older server, and so return a "non supported" error.

[kozlovic]

@ColinSullivan1 @aricart @mcqueary Guys, please have a look since if this is accepted, it will have to be ported in your respective streaming clients.
(the server part will be done when/if this is merged since server needs new protocol vendored).

[ColinSullivan1]

LGTM.

[ColinSullivan1]

Sub is guaranteed to valid at this point?

[kozlovic]

Yes, I started to modify current code to make copies (also for connection), but at this point these values are immutable and both sub and sc are available when used there.

[mcqueary]

LGTM

[coveralls]

Coverage increased (+1.4%) to 88.306% when pulling 0d48ba8 on add_subscription_close into 76fd755 on master.