jmarnold / embeddedmail Goto Github PK

An interesting problem arose when I started using this (otherwise wonderful) library.

What I was getting was intermittent test failures, while running acceptance tests against a IIS hosted website that sent emails using System.Net.Mail.SmtpClient. I finally tracked it down to this: SmtpClient normally pools connections to SMTP servers and close them in its own time, and admittedly this seems to be a good idea for a long-running website.

However, it plays havoc with the EmbeddedSmtpServer because it is started and stopped for every test run, and because the IIS website outlasts the test run, the pooled connection is terminated. When the website goes to send another email (say, on the next test run), it breaks, saying that the connection has been terminated - that's the connection to the EmbeddedSmtpServer in the previous test run.

It seems there are three approaches:

1. The server can tell the client that its disconnecting, closing the connection gracefully. The SMTP spec describes a 421 response, which a server can use to terminate the connection. The idea is that the client would read their TCP connection before attempting more interaction with the server, however this doesn't seem to work for SmtpClient.
2. Use the .Net 4.0 SmtpClient, Disposing after each sent email. This could be done in an automated test environment, leaving the connection pooling to production.
3. Leave the SMTP server going for as long as the connection pools seem to survive. Connection pools seem to live (AFAIK) for less than a minute, so as long as two test runs are at least a minute apart then it seems to be ok. Not exactly fool proof.

The first seemed promising but didn't seem to work, neither of the other two are particularly good solutions.

Nevertheless, I thought I'd document this here for future reference.

Any ideas, thoughts, advise?

The Dispose method of EmbeddedSmtpServer is implemented as

public void Dispose()
{
    Stop();
    // I don't grok the disposal lifecycle for the sockets yet
    Listener.Stop();

    _sessions.Each(x => x.Dispose());
}

but I think it should be more like

public void Dispose()
{
    Stop();
    _sessions.Each(x => x.Dispose());
    Listener.Stop();
}

I have two reasons for this: one theoretical and one practical.

The theoretical reason is that objects should be disposed in the reverse order in which they are created, and Listener is created before each session. In fact (from the point of view of EmbeddedSmtpServer), Listener creates the sockets.

The practical reason is that each session on disposal tries to write to its socket but Listener disposes "the underlying Socket" when its Stop method is called:

The Stop() method also closes the underlying Socket

(I am a bit confused by the use of the singular in that quote. Does each session share the same socket? Not exactly sure about that part.)

Exact details aside, the current order causes an ObjectDisposedException to be thrown. I don't think the order that I suggested will have this problem.

The documentation of StreamReader.ReadLine() says that it returns

The next line from the input stream, or null if the end of the input stream is reached.

My expectation is that _socket.Connected would return true if and only if _reader.ReadLine() would return null. In our fork, which granted is a bit different, this didn't happen recently. I think it was some crazy fluke event. When we expected _reader.ReadLine() to return "QUIT", it returned null instead. Then there was a 28 second delay before it started reading null without delay. At the same time, _socket.Connected kept returning true. Furthermore, when we expect the next SMTP protocol to be QUIT, we have a five second timeout set on the underlying NetworkStream, and this timeout didn't happen either.

Fine...so _socket.Connected and _reader.ReadLine() can be "inconsistent like this". Then we should break out of the while loop if _socket.Connected returns false or if _reader.ReadLine() returns null. I will create a PR to make this change.

RFC 5321, Section 4.5.2. Transparency says in part

• Before sending a line of mail text, the SMTP client checks the first character of the line. If it is a period, one additional period is inserted at the beginning of the line.

• When a line of mail text is received by the SMTP server, it checks the line. If the line is composed of a single period, it is treated as the end of mail indicator. If the first character is a period and there are other characters on the line, the first character is deleted.

Here is the relevant method in EmbeddedMail. I see two problems with this.

First, EmbeddedMail does not remove the extra period added by mail clients when the line begins with a period.

Second, EmbeddedMail ignores white space when checking for the end of mail indicator. If the email body should contain

aa
 .
bb
cc

then a compliant mail client will send

aa
 .
bb
cc
.

and EmbeddedMail will create two messages, one with body

aa

and one with body

cc

Fixing this is easy, but it would conflict with RPs #6 and #8. I will wait for these PRs to be resolved before creating a fix for this issue.

After receiving the body of a message, what is the purpose of creating a new GUID and sending it to the client?

session.WriteResponse(string.Format("250 Ok: queued as {0}", Guid.NewGuid()));

If this GUID were sent more than once or added to the metadata of the received message, then this would make more sense to me. But in its current form, this code is essentially sending random data to the client (unless you consider how the GUID is generated and try to parse out the non-unique parts...which is not a good idea).

The MessageParsingHandler adds the message terminator character . to the message. This is character is part of the SMTP protocol, not part of the message. I will create a PR to fix this.