-
Notifications
You must be signed in to change notification settings - Fork 220
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
Limit maximum number of connections #271
base: master
Are you sure you want to change the base?
Conversation
I'd prefer to leave it up to the library user to implement this feature. |
I just wonder, why there is support for a maximum number of recipients, then. |
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: This has multiple drawbacks:
|
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... |
@mgnsk
|
@emersion
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. |
server.go
Outdated
// limit connections | ||
if s.MaxConnections > 0 && len(s.conns) > s.MaxConnections { | ||
c.Reject() | ||
return nil | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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 thedefer
block is declared after thec.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed again.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
No description provided.