Modularize

[jordanschalm]

Hi, I'm currently working on a project that will require an SMTP server with API hooks for processing received mail, and it sounds like you'd like this project to support that, given the Modularize bounty.
I would be interested in doing a code review and implementing a public API for the package, if that's still something you are interested in adding.

[flashmob]

Hi Jordan! Thanks for offering your help - and welcome to the project!
A lot modularization work has been completed in this PR #16 although the bounty wasn't claimed, therefore the bounty can be kept open since there is always more work that can to be done in this area.
Since the refactor, we now have the ability to develop different backends for saving email that can be switched via the config. Would be interested to get your feedback & suggestions for developing this further. One of the features that would be needed: add the ability of saving the emails in chunks rather than in one go. If there are any other functions / types that need to be added to the backend interface, please feel free to add them.
Would be interested in adding a public API would be too!

[jordanschalm]

Hey, thanks for the quick reply! I wrote up a draft of what the public API could look like. Would love to get some feedback/suggestions.

Guerrilla

// Returns a new instance of Guerrilla with the given config, not yet running.
func New(*Config, Backend) Guerrilla

// Main app object. Can contain multiple servers, and one backend.
// Can delegate things like configuration changes and global events to servers,
// and aggregate info from servers for the web analytics panel down the road
type Guerrilla interface {
	// Starts the app, and all servers in the config.
	// Equivalent to running `guerrillad serve`
	Run()
	// Resets the app's configuration and restarts each server
	Reload(*Config)
}

Server

// Returns a new server instance with the given config, not yet running.
// Sets up TLS etc.
func NewServer(*ServerConfig) Server

type Server interface {
	// Starts the server
	Run()
        // Hot reloads a new configuration
        Reload(*ServerConfig)
}

Client

// Symbolic constant representing client's state
type ClientState int

type Client struct {
	State       ClientState
	Helo        string
	MailFrom    *EmailParts
	RcptTo      []*EmailParts
	Data        string
	ConnectedAt time.Time
	KilledAt    time.Time
	SavedNotify chan int
	ID          int64
	// other fields unexported
}

// Notify client whether save operation succeeded or failed
func (c *Client) Notify(saved bool)

Backend

// Where all received mail is sent. Consumers of the `guerrilla` package would implement `Backend`
// as they see fit (send to RabbitMQ / store in Mongo) or use the built in SQL+Redis backend
// implementation. Then would pass any Backend to `guerrilla.New` when creating an app instance.
type Backend interface {
	// Processes and stores data from a client, notifies success/failure over client's channel when complete.
	Process(*Client)
}

Config

// Config that applies globally to a `guerrilla.Guerrilla` object,
// which may contain multiple `guerrilla.Server`s
type Config struct {
	Servers      []*ServerConfig
	AllowedHosts []string
}

// Config for one server.
type ServerConfig struct {
	IsEnabled       bool
	Hostname        string
	AllowedHosts    []string
	MaxSize         int64
	PrivateKeyFile  string
	PublicKeyFile   string
	Timeout         int
	ListenInterface string
	AdvertiseTLS    bool
	RequireTLS      bool
	MaxClients      int
}

In order to do this, I think it would be a good idea to:

1. Collapse server, config, and util sub-packages into the main guerrilla package. (Maybe modularize isn't the best name for this issue). Reasoning:

• Simplifies imports within the project and limits the possibility of circular imports down the line
• Allows consumers of the guerrilla package to import only one package and have access to the full public API, rather than having to import three sub-packages separately

2. Refactor cmd/guerrilla/serve to be a consumer of the public API (i.e. move lines 93-103 into Guerrilla.Start)

I've done a code review and have made a number of changes on my fork:

• Add ClientState type and symbolic constants for Client.State
• Change Client.ConnectedAt and Client.Killed to be time.Time rather than int64
• Change Client.MailFrom and Client.RcptTo to use EmailParts directly instead of string, and have the server parse these immediately after receiving them rather than after receiving the DATA command
• Add a couple of utility methods to Client: kill and reset
• Found a tiny grammar mistake in the root command long description
• Changed --pidFile CLI flag to --pid-file (not sure whether this is something you guys want, but this is the format of most CLI programs I see)
• Add a couple symbolic constants for the Server.handleClient section
• Remove unnecessary checks of len(input) in Server.handleClient section (x[len(x):] is a safe operation)
• Make Server.Timeout a time.Duration in seconds at initialization
• Change Config.AllowedHosts to []string rather than string with comma-separated elements

It would probably be easiest to incorporate these and get them checked by you guys in the same PR as the API.

Would love to get some feedback on all this. Cheers!

Edit: Could you elaborate a bit on the issue of saving emails in chunks?

[flashmob]

Thanks for the proposal! Just got back from travelling and getting ready to merge in #19 (been working on it while on the road)
The code review makes some good points, and deserves a reward from the pot - please email your BTC address for the reward to be sent. ([email protected]).

About collapsing, didn't realize that the current package layout has these impacts. Will the API be abstracted from the internal working of the server? For example, the API callee might not want to know about SavedNotify chan int - all they will see if that their call will block until saved, or timeout with an error, and actually know that their message was picked up by a channel.

Would an alternative way to do this would be to create a new guerrilla/api package, and then the api package wraps/imports everything it needs, then the consumer would only need to import guerrilla/api package? Also, in case there is a circular dependency (ouch!), would it be valid just to break up rather combining all into one?

To elaborate on saving email in chunks:

The DATA command can buffer a lot of data to memory, so it would be good to save this data out of memory as it's being received. For example, on every 1MB read (or some other boundary), the data chunk could be passed to the backend, saved and then the memory can be freed/recycled (circular buffer perhaps?). Still haven't figured out how to implement this yet, but it would be something that would allow the server to support larger attachments.

[jordanschalm]

My thought on the API is:
The Guerrilla interface is exposed, so that an app can be started with any number of servers by specifying a Config struct and a backend. The operation of the servers themselves is abstracted away from the API. The way the API consumer hooks into the app is by providing a custom backend whose Process method is called for every received email. The way I've been thinking about it, the guerrillad CLI consumes the public API by implementing Backend with the RedisDbBackend and passing it to guerrilla.New, along with the config struct it reads from the config file. So most of the interaction happens within the Process method of the implementation of backend.
API usage something like:

type CustomBackend struct {...}
func (c *CustomBackend) Process(c *guerrila.Client) {
    // Do what you need with the received message
}

backend := &CustomBackend{}
config := readConfigFromFile()
myApp := guerrila.New(backend, config)
myApp.Start()

Moving the api to its own package might be a good solution too. I'll try that out and we can see how it looks.
Just sent you an email this morning. I will try to make a work-in-progress PR tomorrow evening with the work I've done on this so far.