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

[FEAT] document -ERR protocol messages #282

Merged
merged 13 commits into from
Aug 21, 2024
Merged

[FEAT] document -ERR protocol messages #282

merged 13 commits into from
Aug 21, 2024

Conversation

aricart
Copy link
Member

@aricart aricart commented May 31, 2024

No description provided.

@aricart aricart requested review from Jarema and ripienaar May 31, 2024 17:53
@ripienaar
Copy link
Contributor

Once we are happy here we should update https://docs.nats.io/reference/reference-protocols/nats-protocol - and really I think its best that just links to this doc else its almost guaranteed to be out of date.

@Jarema
Copy link
Member

Jarema commented Jun 24, 2024

@aricart I think we are good with this PR.
Let's merge it into the Connection spec.

@aricart aricart requested review from ripienaar and Jarema June 24, 2024 14:32
@aricart aricart requested review from piotrpio and wallyqs July 12, 2024 15:50
@Jarema
Copy link
Member

Jarema commented Jul 15, 2024

@aricart One last change: instead of using those ????????????????? I would user nicer notices like those:
https://github.com/orgs/community/discussions/16925

@ripienaar
Copy link
Contributor

sorry @aricart I accidentally stole your ADR number for another PR :(

@aricart
Copy link
Member Author

aricart commented Jul 23, 2024

I think I got all the changes we were looking for - if we are good we can merge.

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!


The default interval for PING is 2 minutes.

### Error Handling (TODO)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing Stale Connection, client should reconnect after getting this error:

https://github.com/nats-io/nats.go/blob/main/nats.go#L3655-L3665

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roughly in the Go client, it reconnects when receiving either one of these:

  • stale connection
  • max connection exceeded

Does not reconnect and just triggers async callbacks:

  • permissions and auth errors

Then for any other -ERR it will close the connection and not reconnect again:

https://github.com/nats-io/nats.go/blob/main/nats.go#L3647-L3675

[`connection_rate_limit`](https://github.com/nats-io/nats-server/blob/main/server/opts.go#L4557)
is set.

#### Max Payload Violation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clients ougth to close the connection after this one

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure - we need to define a throttling strategy for connections and api requests that makes sense. Clients will just reconnect according to that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the jitter+reconnect interval is not correct.- it needs to back off.

@@ -52,209 +61,426 @@ This method is available in all NATS Server versions.

1. Clients initiate a network connection to the Server.
2. Server responds with [INFO][INFO] json.
3. If Server [INFO][INFO] contains `tls_required` set to `true`, or the client has a tls requirement set to `true`, the client performs a TLS upgrade.
3. If Server [INFO][INFO] contains `tls_required` set to `true`, or the client
has a tls requirement set to `true`, the client performs a TLS upgrade.
Copy link
Collaborator

@scottf scottf Jul 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the behavior going forward, because it's not the current behavior. The current behavior is that the client and server are suppose to match. At least that is the behavior in both Java and .NET V1 when I inherited them 3 years ago.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The server supports the ability of offering non-tis connections.
If it does, and you have a tls configuration on the client, it assumes, you want tis.
If it doesn't it also asumes that.
But if you can specify something like null it implies you don't want tls for the connection (they are opting out).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when the server supports tls and the client uses an URL like nats://connect.ngs.global then it would auto upgrade to use TLS, clients ought to be able to handle that

Copy link
Contributor

@piotrpio piotrpio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@wallyqs wallyqs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ripienaar ripienaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Jarema Jarema merged commit b9a2f7e into main Aug 21, 2024
1 check passed
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

Successfully merging this pull request may close these issues.

6 participants