Changeset 168 in code for trunk

Timestamp:

Mar 27, 2020, 6:17:58 PM (5 years ago)

Author:

contact

Message:

Nuke user.lock

Split user.register into two functions, one to make sure the user is
authenticated, the other to send our current state. This allows to get
rid of data races by doing the second part in the user goroutine.

Closes: ​https://todo.sr.ht/~emersion/soju/22

Location:

trunk

Files:

• downstream.go (modified) (4 diffs)
• user.go (modified) (8 diffs)

trunk/downstream.go

@@ -72 +72 @@
         username    string
         rawUsername string
+        networkName string
         realname    string
         hostname    string
@@ -583 +584 @@
 }
 
-func (dc *downstreamConn) setNetwork(networkName string) error {
-        if networkName == "" {
+func (dc *downstreamConn) authenticate(username, password string) error {
+        username, networkName := unmarshalUsername(username)
+
+        u := dc.srv.getUser(username)
+        if u == nil {
+                dc.logger.Printf("failed authentication for %q: unknown username", username)
+                return errAuthFailed
+        }
+
+        err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password))
+        if err != nil {
+                dc.logger.Printf("failed authentication for %q: %v", username, err)
+                return errAuthFailed
+        }
+
+        dc.user = u
+        dc.networkName = networkName
+        return nil
+}
+
+func (dc *downstreamConn) register() error {
+        if dc.registered {
+                return fmt.Errorf("tried to register twice")
+        }
+
+        password := dc.password
+        dc.password = ""
+        if dc.user == nil {
+                if err := dc.authenticate(dc.rawUsername, password); err != nil {
+                        return err
+                }
+        }
+
+        if dc.networkName == "" {
+                _, dc.networkName = unmarshalUsername(dc.rawUsername)
+        }
+
+        dc.registered = true
+        dc.username = dc.user.Username
+        dc.logger.Printf("registration complete for user %q", dc.username)
+        return nil
+}
+
+func (dc *downstreamConn) loadNetwork() error {
+        if dc.networkName == "" {
                 return nil
         }
 
-        network := dc.user.getNetwork(networkName)
+        network := dc.user.getNetwork(dc.networkName)
         if network == nil {
-                addr := networkName
+                addr := dc.networkName
                 if !strings.ContainsRune(addr, ':') {
                         addr = addr + ":6697"
@@ -600 +644 @@
                         return ircError{&irc.Message{
                                 Command: irc.ERR_PASSWDMISMATCH,
-                                Params:  []string{"*", fmt.Sprintf("Failed to connect to %q", networkName)},
+                                Params:  []string{"*", fmt.Sprintf("Failed to connect to %q", dc.networkName)},
                         }}
                 }
 
-                dc.logger.Printf("auto-saving network %q", networkName)
+                dc.logger.Printf("auto-saving network %q", dc.networkName)
                 var err error
                 network, err = dc.user.createNetwork(&Network{
-                        Addr: networkName,
+                        Addr: dc.networkName,
                         Nick: dc.nick,
                 })
@@ -619 +663 @@
 }
 
-func (dc *downstreamConn) authenticate(username, password string) error {
-        username, networkName := unmarshalUsername(username)
-
-        u := dc.srv.getUser(username)
-        if u == nil {
-                dc.logger.Printf("failed authentication for %q: unknown username", username)
-                return errAuthFailed
-        }
-
-        err := bcrypt.CompareHashAndPassword([]byte(u.Password), []byte(password))
-        if err != nil {
-                dc.logger.Printf("failed authentication for %q: %v", username, err)
-                return errAuthFailed
-        }
-
-        dc.user = u
-
-        return dc.setNetwork(networkName)
-}
-
-func (dc *downstreamConn) register() error {
-        password := dc.password
-        dc.password = ""
-        if dc.user == nil {
-                if err := dc.authenticate(dc.rawUsername, password); err != nil {
-                        return err
-                }
-        } else if dc.network == nil {
-                _, networkName := unmarshalUsername(dc.rawUsername)
-                if err := dc.setNetwork(networkName); err != nil {
-                        return err
-                }
-        }
-
-        dc.registered = true
-        dc.username = dc.user.Username
-        dc.logger.Printf("registration complete for user %q", dc.username)
-
-        dc.user.lock.Lock()
+func (dc *downstreamConn) welcome() error {
+        if dc.user == nil || !dc.registered {
+                panic("tried to welcome an unregistered connection")
+        }
+
+        // TODO: doing this might take some time. We should do it in dc.register
+        // instead, but we'll potentially be adding a new network and this must be
+        // done in the user goroutine.
+        if err := dc.loadNetwork(); err != nil {
+                return err
+        }
+
         firstDownstream := len(dc.user.downstreamConns) == 0
-        dc.user.lock.Unlock()
 
         dc.SendMessage(&irc.Message{

trunk/user.go

@@ -92 +92 @@
         events chan event
 
-        lock            sync.Mutex
         networks        []*network
         downstreamConns []*downstreamConn
@@ -106 +105 @@
 
 func (u *user) forEachNetwork(f func(*network)) {
-        u.lock.Lock()
         for _, network := range u.networks {
                 f(network)
         }
-        u.lock.Unlock()
 }
 
 func (u *user) forEachUpstream(f func(uc *upstreamConn)) {
-        u.lock.Lock()
         for _, network := range u.networks {
                 uc := network.upstream()
@@ -122 +118 @@
                 f(uc)
         }
-        u.lock.Unlock()
 }
 
 func (u *user) forEachDownstream(f func(dc *downstreamConn)) {
-        u.lock.Lock()
         for _, dc := range u.downstreamConns {
                 f(dc)
         }
-        u.lock.Unlock()
 }
 
@@ -149 +142 @@
         }
 
-        u.lock.Lock()
         for _, record := range networks {
                 network := newNetwork(u, &record)
@@ -156 +148 @@
                 go network.run()
         }
-        u.lock.Unlock()
 
         for e := range u.events {
@@ -171 +162 @@
                 case eventDownstreamConnected:
                         dc := e.dc
-                        u.lock.Lock()
+
+                        if err := dc.welcome(); err != nil {
+                                dc.logger.Printf("failed to handle new registered connection: %v", err)
+                                break
+                        }
+
                         u.downstreamConns = append(u.downstreamConns, dc)
-                        u.lock.Unlock()
                 case eventDownstreamDisconnected:
                         dc := e.dc
-                        u.lock.Lock()
                         for i := range u.downstreamConns {
                                 if u.downstreamConns[i] == dc {
@@ -183 +177 @@
                                 }
                         }
-                        u.lock.Unlock()
                 case eventDownstreamMessage:
                         msg, dc := e.msg, e.dc
@@ -221 +214 @@
         })
 
-        u.lock.Lock()
         u.networks = append(u.networks, network)
-        u.lock.Unlock()
 
         go network.run()