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.

Sign up for GitHub

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

Limit maximum number of connections #271

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Limit maximum number of connections #271

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2a4dfab
Limit maximum number of connections
sapmli Sep 12, 2024
22834b4
Fix missing s.locker.lock() and duplicate c.Close()
sapmli Sep 18, 2024
8ff94a7
Fix race condition
sapmli Sep 18, 2024
2ae8ba2
Add unit test
sapmli Sep 18, 2024
94fcc07
revert unnecessary change
sapmli Sep 18, 2024
b1ea4cc
fix test doc
sapmli Sep 18, 2024
6c510f0
fix test doc
sapmli Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ type Server struct {
LMTP bool

Domain string
MaxConnections int
MaxRecipients int
MaxMessageBytes int64
MaxLineLength int
Expand Down Expand Up @@ -151,6 +152,12 @@ func (s *Server) handleConn(c *Conn) error {
}
}

// limit connections
if s.MaxConnections > 0 && len(s.conns) > s.MaxConnections {
c.Reject()
return nil
}
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link

@mgnsk mgnsk Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

@mgnsk mgnsk Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pushed again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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


c.greet()

for {
Expand Down