From abe7d99c30500a248ba971fc14da3acea1b4d130 Mon Sep 17 00:00:00 2001 From: Marco Peereboom Date: Thu, 25 Jul 2019 17:53:59 +0100 Subject: [PATCH 1/5] Disable keepalives in server. Let client reconnect if something fails. --- zkserver/zkserver.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/zkserver/zkserver.go b/zkserver/zkserver.go index a3b237d..7d40944 100644 --- a/zkserver/zkserver.go +++ b/zkserver/zkserver.go @@ -32,7 +32,7 @@ import ( "github.com/companyzero/zkc/zkserver/settings" "github.com/companyzero/zkc/zkutil" "github.com/davecgh/go-spew/spew" - "github.com/davecgh/go-xdr/xdr2" + xdr "github.com/davecgh/go-xdr/xdr2" ) const ( @@ -659,8 +659,7 @@ func (z *ZKS) listen() error { continue } - conn.(*net.TCPConn).SetKeepAlive(true) - conn.(*net.TCPConn).SetKeepAlivePeriod(time.Second) + conn.(*net.TCPConn).SetKeepAlive(false) conn = tls.Server(conn, &config) go z.preSession(conn) From cfbda4a81b51ab6f332dfc43bb1b1f9dfd6d4680 Mon Sep 17 00:00:00 2001 From: Marco Peereboom Date: Thu, 25 Jul 2019 18:09:29 +0100 Subject: [PATCH 2/5] Please linter --- zkclient/completion.go | 1 - 1 file changed, 1 deletion(-) diff --git a/zkclient/completion.go b/zkclient/completion.go index f986140..8a7a07f 100644 --- a/zkclient/completion.go +++ b/zkclient/completion.go @@ -84,7 +84,6 @@ func (z *ZKC) completeNickCommandLine(args []string) { var c string switch len(args) { case 1: - c = "" return case 2: c = args[1] From af81f1eb7e6a5e08e6bfdef63175849ae3d25fa1 Mon Sep 17 00:00:00 2001 From: Marco Peereboom Date: Thu, 1 Aug 2019 12:29:08 +0100 Subject: [PATCH 3/5] Knock account offline when a duplicate ID comes in The server doesn't always seem to detect when a client disappears off the net. This diff adds a mechanism to knock current connections offline if a duplicate ID comes in. This is annoying when the user may accidentally have two zkclients open but it beats being locked out of zkserver for hours at a time. --- zkserver/account/account.go | 33 ++++++++++++----- zkserver/zkserver.go | 72 ++++++++++++++++++++++++++++--------- 2 files changed, 80 insertions(+), 25 deletions(-) diff --git a/zkserver/account/account.go b/zkserver/account/account.go index dd9dbb4..a5bb4d6 100644 --- a/zkserver/account/account.go +++ b/zkserver/account/account.go @@ -18,7 +18,7 @@ import ( "github.com/companyzero/zkc/inidb" "github.com/companyzero/zkc/zkidentity" - "github.com/davecgh/go-xdr/xdr2" + xdr "github.com/davecgh/go-xdr/xdr2" ) const ( @@ -26,6 +26,14 @@ const ( UserIdentityFilename = "user.ini" ) +type ErrAlreadyOnline struct { + err error +} + +func (e ErrAlreadyOnline) Error() string { + return e.err.Error() +} + // Account opaque type that handles account related services. type Account struct { root string // root location of all accounts @@ -315,15 +323,22 @@ func (a *Account) Delete(from [zkidentity.IdentitySize]byte, identifier string) return nil } -func (a *Account) Offline(who [zkidentity.IdentitySize]byte) { - a.Lock() - defer a.Unlock() - +// offline closes open quit channels and deletes an account from the online +// map. This function must be called WITH the mutex held. +func (a *Account) offline(who [zkidentity.IdentitySize]byte) { dn, found := a.online[who] if found { close(dn.quit) - delete(a.online, who) } + delete(a.online, who) +} + +// Offline knocks a user offline. This function must be called WITHOUT the +// mutex held. +func (a *Account) Offline(who [zkidentity.IdentitySize]byte) { + a.Lock() + defer a.Unlock() + a.offline(who) } // Online notifies Account that a user has become available. It reads all @@ -333,12 +348,14 @@ func (a *Account) Online(who [zkidentity.IdentitySize]byte, ntfn chan *Notificat cache := a.accountFile(who, CacheDir) - // make sure we are online a.Lock() _, found := a.online[who] if found { a.Unlock() - return fmt.Errorf("already online: %v ", hex.EncodeToString(who[:])) + return ErrAlreadyOnline{ + err: fmt.Errorf("already online: %v ", + hex.EncodeToString(who[:])), + } } dn := diskNotification{ diff --git a/zkserver/zkserver.go b/zkserver/zkserver.go index 7d40944..eab4a18 100644 --- a/zkserver/zkserver.go +++ b/zkserver/zkserver.go @@ -61,7 +61,24 @@ type RPCWrapper struct { Identifier string } +type sessionContext struct { + ntfn chan *account.Notification + writer chan *RPCWrapper + quit chan struct{} + kx *session.KX + rids string + tagStack *tagstack.TagStack + + // protected + sync.Mutex + tagMessage []*RPCWrapper +} + type ZKS struct { + sync.Mutex + sessions map[string]*sessionContext + + // Not mutex entries *debug.Debug account *account.Account settings *settings.Settings @@ -199,7 +216,7 @@ func (z *ZKS) sessionWriter(sc *sessionContext) { func (z *ZKS) sessionNtfn(sc *sessionContext) { defer func() { - z.T(idS, "sessionNtfn exit: %v", sc.rids) + z.Dbg(idS, "sessionNtfn exit: %v", sc.rids) // close underlying connection in order to fail read sc.kx.Close() @@ -272,20 +289,6 @@ func (z *ZKS) sessionNtfn(sc *sessionContext) { } } -type sessionContext struct { - ntfn chan *account.Notification - writer chan *RPCWrapper - quit chan struct{} - //done chan bool - kx *session.KX - rids string - tagStack *tagstack.TagStack - - // protected - sync.Mutex - tagMessage []*RPCWrapper -} - // handleSession deals with incoming RPC calls. For now treat all errors as // critical and return which in turns shuts down the connection. func (z *ZKS) handleSession(kx *session.KX) error { @@ -309,15 +312,48 @@ func (z *ZKS) handleSession(kx *session.KX) error { // register identity err := z.account.Online(rid, sc.ntfn) if err != nil { + // If the account is already online we are knocking it offline. + // This may be annoying for users that have two clients open + // but it fixes the issue where phantom server connections + // remain online preventing the client from connecting to the + // server altogether. + if _, ok := err.(account.ErrAlreadyOnline); ok { + z.Dbg(idS, "handleSession forced offline: %v", rids) + z.Lock() + oldSc, ok := z.sessions[rids] + if ok { + // Closing the connection should knock + // everything offline. + oldSc.kx.Close() + delete(z.sessions, rids) + } else { + // XXX wtf + z.Unlock() + return fmt.Errorf("handleSession: account "+ + "online without a session %v", rids) + } + z.Unlock() + } + + // Regardless of the failure we return an error in order to + // give the server the opportunity to close the connection and + // settle down. return fmt.Errorf("handleSession: %v %v", rids, err) } + + // mark session online + z.Lock() + z.sessions[rids] = &sc + z.Unlock() + z.Dbg(idS, "handleSession account online: %v", rids) // populate identity in directory if z.settings.Directory { err := z.account.Push(rid) if err != nil { - z.Dbg(idS, "handleSession: Push(%v) = %v", rids, err) + return fmt.Errorf("handleSession: Push(%v) = %v", + rids, err) } } @@ -670,7 +706,9 @@ func (z *ZKS) listen() error { } func _main() error { - z := &ZKS{} + z := &ZKS{ + sessions: make(map[string]*sessionContext), + } // flags and settings var err error From feab82e906a319b89cd9831cac0680013f4cedca Mon Sep 17 00:00:00 2001 From: Marco Peereboom Date: Thu, 1 Aug 2019 12:51:46 +0100 Subject: [PATCH 4/5] Oops, make sure to delete session on close --- zkserver/zkserver.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/zkserver/zkserver.go b/zkserver/zkserver.go index eab4a18..652a436 100644 --- a/zkserver/zkserver.go +++ b/zkserver/zkserver.go @@ -368,6 +368,11 @@ func (z *ZKS) handleSession(kx *session.KX) error { z.account.Offline(rid) + // mark session offline + z.Lock() + delete(z.sessions, rids) + z.Unlock() + z.Dbg(idS, "handleSession exit: %v", rids) }() From d38543e8f7cd830a935ccb8aab09374b7c01667b Mon Sep 17 00:00:00 2001 From: Marco Peereboom Date: Tue, 6 Aug 2019 16:12:49 +0100 Subject: [PATCH 5/5] @dajohi requests --- zkserver/zkserver.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/zkserver/zkserver.go b/zkserver/zkserver.go index 652a436..91887c8 100644 --- a/zkserver/zkserver.go +++ b/zkserver/zkserver.go @@ -320,14 +320,13 @@ func (z *ZKS) handleSession(kx *session.KX) error { if _, ok := err.(account.ErrAlreadyOnline); ok { z.Dbg(idS, "handleSession forced offline: %v", rids) z.Lock() - oldSc, ok := z.sessions[rids] - if ok { + if oldSc, ok := z.sessions[rids]; ok { // Closing the connection should knock // everything offline. oldSc.kx.Close() delete(z.sessions, rids) } else { - // XXX wtf + // This should not happen. z.Unlock() return fmt.Errorf("handleSession: account "+ "online without a session %v", rids)