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

Allow a Hook to reload an individual client session #402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

snej
Copy link
Contributor

@snej snej commented May 10, 2024

In a clustered environment, client connections are distributed among multiple Server instances on different machines. After a client disconnects, leaving behind a persistent session state, its next login is likely to be on a different node. Because of this, in such a setup an individual Server instance should only keep Client instances corresponding to online client connections, and it should be able to reload an individual client's state (presumably from persistent storage) when that client reconnects.

This commit adds support for such an environment by adding a new hook StoredClientByID. An implementation finds and returns any persistent client data for a given session ID. In practice the only necessary information turned out to be the saved subscriptions and in-flight ack messages. The hook also returns the prior 'Remote' property since the server logs that.

The Server method inheritClientSession is extended to call this hook if there is no matching in-memory Client session. If the hook returns session data, it installs it into the Client object in the same way as the existing code.

At the end of the Server method attachClient, after disconnection, the existence of a StoredClientByID hook is checked; if present, the method expires the Client instance so it won't hang around in memory and so the next connection will go to the hook to reload state.

In a clustered environment, client connections are distributed among
multiple Server instances on different machines. After a client
disconnects, leaving behind a persistent session state, its next
login is likely to be on a different node. Because of this, in such a
setup an individual Server instance should only keep Client instances
corresponding to online client connections, and it should be able to
reload an individual client's state (presumably from persistent
storage) when that client connects.

This commit adds support for such an environment by adding a new hook
`StoredClientByID`. An implementation finds and returns any
persistent client data for a given session ID. In practice the only
necessary information turned out to be the saved subscriptions and
in-flight ack messages. The hook also returns the prior 'Remote'
property since the server logs that.

The Server method `inheritClientSession` is extended to call this hook
if there is no matching in-memory Client session. If the hook returns
session data, it installs it into the Client object in the same way
as the existing code.

At the end of the Server method `attachClient`, after disconnection,
the existence of a `StoredClientByID` hook is checked; if present,
the method expires the Client instance so it won't hang around in
memory and so the next connection will go to the hook to reload
state.
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9038356648

Details

  • 24 of 68 (35.29%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.7%) to 97.934%

Changes Missing Coverage Covered Lines Changed/Added Lines %
hooks.go 0 15 0.0%
server.go 24 53 45.28%
Totals Coverage Status
Change from base Build 8892157277: -0.7%
Covered Lines: 6163
Relevant Lines: 6293

💛 - Coveralls

Copy link
Collaborator

@mochi-co mochi-co left a comment

Choose a reason for hiding this comment

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

Hi @snej ! Thanks for the PR, my apologies for the delay I've been caught up with work and life stuff. I'm into the general idea, but I think we need to make a few changes to the implementation to avoid breaking the rules about hooks - they should operate totally independently from the core server code. We can add new hooks and change signatures to reach this, however. See my comments.

Thanks very much :)

@@ -485,6 +485,11 @@ func (s *Server) attachClient(cl *Client, listener string) error {
expire := (cl.Properties.ProtocolVersion == 5 && cl.Properties.Props.SessionExpiryInterval == 0) || (cl.Properties.ProtocolVersion < 5 && cl.Properties.Clean)
s.hooks.OnDisconnect(cl, err, expire)

if s.hooks.Provides(StoredClientByID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have mixed feelings about this. I like the overall idea of the implementation but I'm not sure we should be adding any hook-specific logic to the server code as it defeats the objective of having hooks. In this case the hook is mostly being used as a feature flag, whereas the hook logic should be stored inside the hook.

Maybe a better option here would be to enhance OnDisconnect so that it can overwrite the value of expire:

expire = s.hooks.OnDisconnect(cl, err, expire)

and modify the method signature accordingly:

func (h *Hook) OnDisconnect(cl *mqtt.Client, _ error, expire bool) bool {

@@ -596,6 +601,42 @@ func (s *Server) inheritClientSession(pk packets.Packet, cl *Client) bool {
return true // [MQTT-3.2.2-3]
}

// Look up a stored client that's not in memory yet:
if s.hooks.Provides(StoredClientByID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same issue here. Possible solution would be to add this logic to the OnSessionEstablish hook and overwrite the values of Client.State directly, but without adding the client to the server. This will avoid the client being picked up in inheritClientSession and overwriting the new networked client values.

@@ -1669,6 +1694,29 @@ func (s *Server) loadClients(v []storage.Client) {
}
}

// newClientFromStorage creates a Client from a storage.Client.
func (s *Server) newClientFromStorage(c *storage.Client) *Client {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not against it, but what's the intention behind extracting this into an additional method if it's only called from the same place?

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.

3 participants