Limit maximum number of connections

[sapmli]

No description provided.

[emersion]

I'd prefer to leave it up to the library user to implement this feature.

[sapmli]

I just wonder, why there is support for a maximum number of recipients, then.
This could be easily implemented in client sessions as well.
And for me, go-smtp is especially responsible for connection management.
But of course, I will fully respect your decision.

[sapmli]

Could you please roughly describe to you would implement this in the backend/session in an elegant way?

After doing this, I found that it's not a good design:
Since the Backend interface only has the NewSession() method and is not called back for closed connections, a session (which are best isolated from each other) now has to be made aware of other sessions, by something like a shared waitgroup or an atomic counter. Then, we need to apply Add() in the backend, and Done() in the session's Logout() method.

This has multiple drawbacks:

• It spreads the implementation to multiple places which needs to stay in sync
• Using Add() and Done() can - for whatever reason - drift over time. Using a single len() is a more stable solution
• The library is still doing the greeting and session init stuff, which could be avoided, especially under heavy load
• The c.Reject() method is dead code, until someone uses it in client implementations

[mgnsk]

You could use https://pkg.go.dev/golang.org/x/net/netutil#LimitListener.

[sapmli]

You could use https://pkg.go.dev/golang.org/x/net/netutil#LimitListener.

Thank you! I'll try that. Though it's not the SMTP way of things...

[sapmli]

@mgnsk
Hmm, I think the LimitListener approach has two downsides:

1. It does not advocate the correct SMTP error code 421. Clients would get an net.OpError, or an io timeout. Of course, this is probably considered something temporary, and the retry is hopefully backed off, but it's not as clear as an SMTP code (and its message).
2. It works only for one listener. If you have more than one listener, e.g. for STARTTLS and TLS, it does not take the other listener into account. Applying two limited listeners might end up in double the allowed usage.

[sapmli]

@emersion
There is one more argument against doing it in the backend:
Before the session is passed to the backend, the greeting is already sent, and the EHLO is parsed from the SMTP client:

220 mail.example.host ESMTP Service Ready
EHLO ...

in which "ESMTP Service Ready" is misleading at best, if not "wrong".

So, I strongly recommend to implement this in the go-smtp library, since it's the perfect spot to do so and IMO delivers the best experience and follows the most common SMTP practice.
Please re-think your decision.

[mgnsk]

If you're going to access s.conns, you must do it safely:

diff --git a/server.go b/server.go
index 4cccbe0..95d325f 100644
--- a/server.go
+++ b/server.go
@@ -128,10 +128,22 @@ func (s *Server) Serve(l net.Listener) error {
 }
 
 func (s *Server) handleConn(c *Conn) error {
+	maxConnsExceeded := false
+
 	s.locker.Lock()
-	s.conns[c] = struct{}{}
+	if s.MaxConnections > 0 && len(s.conns) >= s.MaxConnections {
+		maxConnsExceeded = true
+	} else {
+		s.conns[c] = struct{}{}
+	}
 	s.locker.Unlock()
 
+	// limit connections
+	if maxConnsExceeded {
+		c.Reject()
+		return nil
+	}
+
 	defer func() {
 		c.Close()
 
@@ -152,12 +164,6 @@ func (s *Server) handleConn(c *Conn) error {
 		}
 	}
 
-	// limit connections
-	if s.MaxConnections > 0 && len(s.conns) > s.MaxConnections {
-		c.Reject()
-		return nil
-	}
-
 	c.greet()
 
 	for {

The main issues to keep in mind:

• Safe concurrent access to s.conns.
• Avoid writing to s.conns if it's full.
• Write to connection outside of the lock.
• Don't close the connection twice.

[mgnsk]

c.Reject() already calls c.Close(). That's why it's before the defer block.

[mgnsk]

I'd recommend writing a test for it, but I'm not sure about the test implementation. I see that there is a general testServer function in server_test.go but it only dials a single connection. To test the connection limit, you should dial multiple connections, assert that the first one replies with the greeting and the exceeded one replies with the failure code.

You should also look at how other mail servers implement this and how it plays with go's net/smtp.NewClient(). I'd expect smtp.NewClient() to return the 421 error since it reads the first line from server.

[sapmli]

I just checked c.Reject() an deleted my comment. :-)

[mgnsk]

Thanks for the correction. Please note that this version has a bug, though: It does never call c.Close() because the defer block is declared after the c.Reject(); return nil.

You might be right, in order to preserve the original server behaviour (always a defer c.Close() before attempting any IO on the connection), there should be a defer c.Close() somewhere before c.Reject() so the reject can be after the defer block (the map won't contain the connection anyway). Perhaps also SetDeadline should be used for the reject.

[mgnsk]

Unfortunately your solution has a race condition. Multiple goroutines may check that the limit has not been exceeded, then proceed to exceed the limit. The read and write on s.conns map should happen under the same lock.

[sapmli]

Yes, I know. I thought I'd prefer code readability over total correctness,
but probably you're right, there's no way around it.

[sapmli]

pushed again.

[sapmli]

I think I'm done with the test. Please take a look.

[mgnsk]

Looks fine to me, it's up to @emersion to make the decision.