Track closed connections and reason for closing #692

derekcollison · 2018-06-26T01:04:38Z

This PR allows the server to maintain state on closed connections. Uses a fixed size ring buffer that is configurable. Monitoring endpoints now can return closed connections via /connz?state=closed and will report when they closed and why.

Adresses #520 and #663

Signed-off-by: Derek Collison derek@nats.io

/cc @nats-io/core

Signed-off-by: Derek Collison <derek@nats.io>

coveralls · 2018-06-26T01:10:09Z

Coverage decreased (-0.2%) to 92.431% when pulling ec8e263 on closed into e155332 on master.

kozlovic

I believe that there is an error in ConnInfo.fill() where you don't use the passed nc. Also, the reason for the close we would want to report may not be the one that is recorded due to error detection lower in the stack.

kozlovic · 2018-06-26T01:29:14Z

server/monitor.go

+		return ConnOpen, nil
+	}
+	switch strings.ToLower(str) {
+	case "open":


should this be opened since you have closed (or add both open, opened and close, closed.

I think open is correct since it means perpetual vs closed.

kozlovic · 2018-06-26T01:29:49Z

server/monitor.go

+func (reason ClosedState) String() string {
+	switch reason {
+	case ClientClosed:
+		return "Client"


Should this be Closed instead or Client Closed?

I had it as Client Closed, but thought it read better looking at the JSON as reason for closed is Client.

kozlovic · 2018-06-26T01:33:24Z

server/ring.go

+
+// Fixed sized ringbuffer for closed connections.
+type closedRingBuffer struct {
+	total uint64


Would that make more sense to add a mutex in that object and so we don't need server lock for that? When I create a new object like that, I tend to favor that object having its own lock so we can reduce lock contention. Especially this is a singleton, so adding a mutex should not be costly mem wise.

I thought about that too, but decided its very fast and will only hold for time to copy. I was thinking about preallocating the structs too. I could put a lock around it.

kozlovic · 2018-06-26T01:34:25Z

server/server.go

@@ -834,6 +838,35 @@ func (s *Server) createClient(conn net.Conn) *client {
 	return c
 }

+// This will save off a closed client in a ring buffer such that
+// /connz can inspect. Useful for debugging, etc.
+func (s *Server) saveClosedClient(c *client, nc net.Conn, reason ClosedState) {


This could be a receiving of the ring buffer object (especially if we use a dedicated lock there)

Yes it could. Feels like a server verb though that just uses the ring buffer. We do the time stamp etc.

kozlovic · 2018-06-26T01:37:38Z

server/monitor.go

+	ci.InMsgs = atomic.LoadInt64(&client.inMsgs)
+	ci.InBytes = atomic.LoadInt64(&client.inBytes)
+
+	// If the connection is gone, too bad, we won't set TLSVersion and TLSCipher.


I believe you should use nc down below, not client.nc...

Agree will fix.

kozlovic · 2018-06-26T01:51:30Z

server/server.go

@@ -786,7 +790,7 @@ func (s *Server) createClient(conn net.Conn) *client {
 		if err := conn.Handshake(); err != nil {
 			c.Errorf("TLS handshake error: %v", err)
 			c.sendErr("Secure Connection - TLS Required")
-			c.closeConnection()
+			c.closeConnection(TLSHandshakeError)


Humm... I tried to connect a non TLS client to a TLS server and I get Write Error, not TLS Handshake Error. I believe that since we call sendErr(), it is possible that flushOutbound() gets an error because the client will also close the connection (after receiving the error), so the actual close of the connection occurs there, with WriteError. That may then not prove that useful.

Non TLS client won't try to handshake will it? I will look into it though, agree we want the correct reason.

No they won't.

Actually, what happened is that I was connecting without TLS, so the client when getting the INFO fails the connect and says that secure connection is required. The server when getting into the Handshake will fail and send the error, but since the connection is closed it will get a write error, so that kind of makes sense.

But, I just tried with a client connection with TLS but without the CA root cert so it won't be able to verify the server certificate, and I still get the Write Error as a reason.

ok I will fix and add a test, thanks.

derekcollison · 2018-06-26T03:31:23Z

I also just found a bug with closed connections and optional Subs (and AuthorizedUser). Once set it stays set, will fix in the am.

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic · 2018-06-27T00:15:01Z

server/closed_conns_test.go

@@ -267,7 +267,7 @@ func TestClosedMaxPayload(t *testing.T) {

 func TestClosedSlowConsumerWriteDeadline(t *testing.T) {
 	opts := DefaultOptions()
-	opts.WriteDeadline = 10 * time.Microsecond // Make very small to trip.
+	opts.WriteDeadline = 10 * time.Millisecond // Make very small to trip.


Probably not enough. Bumping it to 30ms in TestWriteDeadline for very same reason.

(it was originally at 1 ms though), so that may be ok...

Seems to pass on travis..

But we can change it..

kozlovic · 2018-06-27T00:31:14Z

LGTM

Track closed connections and reason for closing

ec8e263

Signed-off-by: Derek Collison <derek@nats.io>

derekcollison requested a review from kozlovic June 26, 2018 01:04

kozlovic requested changes Jun 26, 2018

View reviewed changes

derekcollison added 7 commits June 25, 2018 20:32

Use local variable of nc

11c53ce

Signed-off-by: Derek Collison <derek@nats.io>

cluster should be empty when not defined

0c0dd92

Signed-off-by: Derek Collison <derek@nats.io>

Make sure closed connection with options are race safe

e1058d4

Signed-off-by: Derek Collison <derek@nats.io>

megacheck

4a18dae

Signed-off-by: Derek Collison <derek@nats.io>

Wait for all closed connections before starting

f9f478b

Signed-off-by: Derek Collison <derek@nats.io>

Test for closed state being correct

57e5789

Signed-off-by: Derek Collison <derek@nats.io>

Slightly higher so publisher can connect

81a1b02

Signed-off-by: Derek Collison <derek@nats.io>

kozlovic reviewed Jun 27, 2018

View reviewed changes

kozlovic approved these changes Jun 27, 2018

View reviewed changes

derekcollison merged commit 63b0e63 into master Jun 27, 2018

derekcollison deleted the closed branch June 27, 2018 00:32

This was referenced Jun 27, 2018

Provide additional information about slow consumers #520

Closed

improved logging in nats to nats connections #622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track closed connections and reason for closing #692

Track closed connections and reason for closing #692

derekcollison commented Jun 26, 2018

coveralls commented Jun 26, 2018 •

edited

Loading

kozlovic left a comment

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

kozlovic Jun 26, 2018

derekcollison Jun 26, 2018

derekcollison commented Jun 26, 2018 •

edited

Loading

kozlovic Jun 27, 2018

kozlovic Jun 27, 2018

derekcollison Jun 27, 2018

derekcollison Jun 27, 2018

kozlovic commented Jun 27, 2018

Track closed connections and reason for closing #692

Track closed connections and reason for closing #692

Conversation

derekcollison commented Jun 26, 2018

coveralls commented Jun 26, 2018 • edited Loading

kozlovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekcollison commented Jun 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kozlovic commented Jun 27, 2018

coveralls commented Jun 26, 2018 •

edited

Loading

derekcollison commented Jun 26, 2018 •

edited

Loading