Handle expired clients in server.loadClients().

[werbenhu]

I encountered a problem similar to #339 . If the broker is forcefully shut down, and clients have the clean flag set, after restarting the broker and calling loadClients(), these expired clients will be retained indefinitely if the client is no longer connected.

It is necessary to handle expired clients in server.loadClients(). Call OnDisconnect() to trigger the hook for clearing persistent clients. Also, if a client has expired, there is no need to retain this client in server.Clients.

[coveralls]

Pull Request Test Coverage Report for Build 7128618616

• 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.001%) to 98.913%

---

Totals
Change from base Build 7076132597:	0.001%
Covered Lines:	5462
Relevant Lines:	5522

---

💛 - Coveralls

[mochi-co]

Makes sense to me, and a good solution. For me, it's approved! Thank you for finding this @werbenhu!

[x20080406]

I think this change make the logic become too difficult. The loaded client should be invoke the Stop method only.

With this commit, there will get some issues.

expire := (cl.Properties.ProtocolVersion == 5 && cl.Properties.Props.SessionExpiryInterval == 0) || (cl.Properties.ProtocolVersion < 5 && cl.Properties.Clean)
s.hooks.OnDisconnect(cl, packets.ErrServerShuttingDown, expire)
if expire {
	cl.ClearInflights()  // inflights not recoverd
	s.UnsubscribeClient(cl)  // subscription not recoverd
        // there couldn't find clients after loadClients in the `readStore` method,
} else {
	s.Clients.Add(cl)
}

[werbenhu]

@x20080406 If the client has already expired, it would be reasonable to clear its ClearInflights and subscriptions. There would be no need for recovery at that point.

Now that 'cl' has been deprecated, and you have obtained all the necessary information from it, what is the significance of keeping it continued in 'Clients'?

Calling 'stop()' can also resolve the issue. However, it follows the same final processing logic, and it adds two redundant operations: adding the client to 'Clients' and then removing it.

[mochi-co]

Released in v2.4.3
This is a preventative fix to solve the current issue as it's better than having bugs, however I am happy for @x20080406 and @werbenhu for you to please keep discussing the optimal solution to replace this one if necessary.

[thedevop]

@x20080406 @werbenhu @mochi-co

s.NewClient will start the WriteLoop in a Go routine for non-inline clients:

server/server.go

Lines 231 to 239 in 624dde0

	if inline { // inline clients bypass acl and some validity checks.
	cl.Net.Inline = true
	// By default, we don't want to restrict developer publishes,
	// but if you do, reset this after creating inline client.
	cl.State.Inflight.ResetReceiveQuota(math.MaxInt32)
	} else {
	go cl.WriteLoop() // can only write to real clients
	}

This will only stop if cl.Stop or cl.State.cancelOpen is called.

[werbenhu]

@thedevop I found this too, and I will submit a new pull request to fix it.