search

flashmob / go-guerrilla

Mini SMTP server written in golang
MIT License
2.79k stars 366 forks source link

Modularize - Ability for server to be used as a package #27

Closed jordanschalm closed 7 years ago

jordanschalm commented 7 years ago

For #20

flashmob commented 7 years ago

Thanks. Code looks very neat. Well, currently the stuck on https://github.com/flashmob/go-guerrilla/pull/19 - so would merge this first

Some feedback

There's some imports still pointing to your fork (serve.go)

parseHeaders() - maybe parsing headers could be left to the backends? If possible, looking to defer as much work as possible to the backends, or even further down the stack. Would also like to avoid regex where possible. The previous implementation used a simple string compare to find the subject only and stopped scanning once found it, but full header parsing might be better to avoid until later?

parseHeaders() - not all emails can have headers, and data can be unpredictable. The code would panic if the map was nil.

parseHeaders() - to detect header start / end, would it be easier to get a slice like this: headers := [0:strings.Index("\r\n\r\n")] (of course bounds checking before).

For parsing headers, these readers may be useful https://golang.org/pkg/net/textproto/ rather than the http package.

ClientStartTLS - found a bug that would cause an infinite loop if client closes connection during negotiation. (This was already there before)

ClientStartTLS - according to rfc3207, it should not respond with any 220 message after TLS negotiation, but go into expecting EHLO from the client. Your mod to reset the client is correct.

ClientStartTLS - If TLS negotiation failed, it might as well go back to command state without any additional message, the client can decide whenever to continue or not since it would know if TLS failed or not. (Interpreting rfc3207)

So... Committed some fixes to this PR & took the server for a spin on production! Hmm.. It didn't go to well, after a few minutes, it started having trouble with "too many open file" errors and hogging all cpu. Had to shut it down and current master is handling the same traffic with no issues. Not sure what is causing it yet? Perhaps stuck in a loop. There were a lot of "Error reading data" messages before it went into "too many open file" mode..

jordanschalm commented 7 years ago

Thanks for the feedback. I'll move the header parsing to a util function and leave it to the backend and try to figure out what is causing those errors.

flashmob commented 7 years ago

Still not sure what is causing the above problem. It looks like the number of connected clients fills up quickly to the max (5000) and then the "too many open file" errors come in this form:

time="2016-12-13T16:47:28Z" level=debug msg="Waiting for a new client. Client ID: 5387"
time="2016-12-13T16:47:28Z" level=info msg="Error accepting client" error="accept tcp 192.99.19.220:25: accept4: too many open files"

Noticed that there is a bug when in DATA state, if the host is not allowed, it will still go to save the email & respond with two messages, 550 Error: Host not allowed: test.com and 250 OK : queued as 309ea3da1721ec4e96da2e7a0c082275

Making some fixes, will commit and send them up soon, then test again

flashmob commented 7 years ago

Still getting the errors after the bug fixes. Slight correction to the above message: server fills up to about 1000 clients before going into weird state. Got a feeling that the backend can't keep up? Looking at the guerrilla_db_redis.go file, it looks like the notification channel has been moved there, may be something with this.

flashmob commented 7 years ago

Hmm, backend seems OK so far. Maybe it's something related to this? https://ttboj.wordpress.com/2015/07/27/golang-parallelism-issues-causing-too-many-open-files-error/

jordanschalm commented 7 years ago

I think my changes to the backend config loading were the problem. NumWorkers was always set to 0 so no workers were created for the backend -- created deadlock pretty quickly. It's fixed now in my tests. 🎉

flashmob commented 7 years ago

Ahh, my testing account has ulimit of 1024! ulimit -n Will ramp it up and see what happens.

Thanks for the changes!

flashmob commented 7 years ago

Increased the limits & tested with the following: timeout: 10 sec backend workers: 9 max clients: 5000

it appears stable now!

Perhaps the server should check ulimit and warn if ulimit < max_clients...

flashmob commented 7 years ago

Thanks for updates, including the Readme update - was going to ask you about it, but looks like we're on the same page!

We should be ready to merge this soon?

Regarding the c Client, what would you say if we placed the c.Data, c.MailFrom, c.RcptTo, c.Helo, c.Address, c.TLS, in their own struct, eg. type MailData struct. The reason is that the backend should not depend on the Client, mostly because it will make it easier to to work with it in the future, since the backends will not hold the pointer to Client. It may also make optimizations easier in the future, eg. c.Data to could be a bytes buffer instead of a string, and we don't have to worry about changing the API signiture if the Client changes. What do you say?

jordanschalm commented 7 years ago

I agree. Separating public Client data into its own struct is a good idea. I can do that later today. After that it should be ready to merge.

flashmob commented 7 years ago

Thanks! That will be another bounty earned!

Before merging, a thorough test will be done on Guerrilla Mail production.

Btw, In the future, considering to change Data to a []byte - the reason is because string assumes text, however Data is just a raw chunk of bytes that does not really represent text, eg it could be 8bitmime. The other reason, it would be more efficient to use with Reader / Writer to avoid the string to []byte conversion. What do you think?

jordanschalm commented 7 years ago

If we're leaving the interpretation of the DATA to backends, I think leaving Data as byte array is the right choice. It might make pulling the subject header out more complicated though.

flashmob commented 7 years ago

Thanks. That was quick!

Regarding MailData, do you think this would be the best name, or could there be more concise alternatives? Eg. Envelope or any other ideas?

flashmob commented 7 years ago

Last push fixed a bug that caused start tls to be advertised even though when it was off. Also, STARTTLS command will be ignored if it is disabled (lots of clients try it despite not advertised). Getting more stable to merge. Is there anything else you wanted to add?

jordanschalm commented 7 years ago

Nice. I would like to come up with a better name than MailData. You're right, that is pretty vague...

Maybe one of: Envelope Parcel Message MailPayload Email

And do you want to change EmailParts to EmailAddress or MailAddress? I think it's a bit clearer. Email is such an overloaded term

jordanschalm commented 7 years ago

I'm happy to merge now, unless you want to change the Envelope/EmailParts naming.

flashmob commented 7 years ago

Envelope would be a good metaphor. Good point on the EmailParts, EmailAddress is better. Also, in the client struct, Address was changed to RemoteAddress, just to make things clear.

So testing is going great! This branch has been running on Guerrilla Mail for the last 12 hours, usually middle of the week is the most busiest time for a battle test.

Looks like ready to go. Thanks for another bounty earned!