GSoC: finagle-smtp - initial #287

suncelesta · 2014-06-25T19:36:33Z

This is initial version of SMTP client for Finagle, which I am working on during GSoC'14.
For now it supports sending independent commands to SMTP server and sending a simple text email to one ore more addresses (without cc, bcc and reply-to). Usage is described in README.md.

Handling only text messages; no tests

Add simple support for Finagle 6 client API

Added the ability to send any existing SMTP command + two types of client: one just sending emails and the other sending discrete commands

+ client dispatcher + transporter + reply decoder

Added handling of server greeting and multiline extension list + command encoder + refactoring of request and reply types

+Duplicating leading dot +Adding headers (Not working with copies for now) +Some different factories for EmailMessage

Added connection phase, removed Greeting and handlers constructing it

Commands are dispatched in the correct order Corrected MAIL FROM command + every recipient goes to its own RCPT TO command

Got rid of Result in favor of Reply in Smtp and Unit in SmtpSimple (so when simply sending one email, it results either in a Future.Done or a failed Future in case of exception)

Extended multiline support on all reply types, removed unused helper classes like ComposedRequest and EmptyReply

Added unit tests for filters Created some inner packages, moved Smtp out of package smtp

Tests for SmtpClientDispatcher and ReplyDecoder

+ In case of error in multiline reply the resulting reply is now multiline, holding previously received information + A test case for that matter + Session state is now reset before sending email (for the ability to try again in case of error) + Cleanup of SimpleSmtpClient (renamed to Example) + README with usage information

fix README to appropriate markdown format and syntax highlighting

selvin · 2014-06-26T18:52:27Z

I see that this build failure is not a result of these proposed code changes

error sbt.ResolveException: unresolved dependency: com.twitter#util-core_2.9.2;6.18.0: not found

at https://travis-ci.org/twitter/finagle/jobs/28439637

and http://mvnrepository.com/artifact/com.twitter/util-core_2.9.2 does not have 6.18.0

Any ideas on how to proceed?

+support for cc, bcc, reply-to +EmailBuilder for composing emails +features for creating MailingAddress and retreiving mailbox string from it

mosesn · 2014-08-06T19:30:46Z

finagle-smtp/src/main/scala/com/twitter/finagle/smtp/transport/ReplyDecoder.scala

+        val second = rep(1)
+        val third = rep(2)
+
+        //Standart reply: three-digit-code SP info


Standart => Standard

mosesn · 2014-08-06T19:38:23Z

This looks generally good, but there are a few pieces that seem like they might fit better in the dispatcher than where they are right now. I'd be interested in seeing some more end to end tests. The testing could definitely be a little more thorough.

However, most of my suggestions are nitpicks. In general, this looks quite good! Great job. 👍

mosesn · 2014-08-07T18:54:27Z

finagle-smtp/src/main/scala/com/twitter/finagle/smtp/transport/ReplyDecoder.scala

+    if (buf == null) null
+    else buf match {
+      case cb: ChannelBuffer => {
+        val rep = cb.toString(CharsetUtil.UTF_8)


should this be ascii?

Yes, I must have missed that one, thanks.

+ Fixed style + EmailBuilder implements EmailMessage + Aggregating multiline requests moved to the dispatcher

mosesn · 2014-08-08T20:51:09Z

finagle-smtp/src/main/scala/com/twitter/finagle/Smtp.scala

+              override def close(deadline: Time): Future[Unit] = {
+                if (service.isAvailable)
+                  service(Request.Quit)
+                service.close(deadline)


I don't think service(Request.Quit) before service.close(deadline) will work, because we want to close even if we can't Quit properly, but maybe ensure would be the right semantic. Can you use wireshark and check that the tcp connection is actually being torn down when you expect it to be torn down?

According to wireshark:

QUIT is sent

Server responds

Server closes connection

Client closes connection

Seems like what should be expected.

Wait, I thought you were saying that it wasn't behaving properly in the dispatcher? I think I got confused as to what was going on . . . I was trying to reply to a message you posted that said,

Yes, when testing with SMTP server stub and looking at its logs, I found out that this request is not actually sent from dispatcher, so I sought the way to enforce service(Request.Quit) before service.close(deadline)

but I couldn't find the actual message (github swallowed it maybe) so I just put it here.

Is the behavior correct now then?

Oh, sorry, I thought you were asking to check the behavior it has right
now, not with sending QUIT in the dispatcher. I'll check the latter, but I
can say that the server doesn't receive the request in that case, because
in its logs the connection is torn down by client and QUIT isn't received.

2014-08-11 18:45 GMT+04:00 Moses Nakamura [email protected]:

In finagle-smtp/src/main/scala/com/twitter/finagle/Smtp.scala:

* Constructs an SMTP client.

* Upon closing the connection this client sends QUIT command;

* it also performs dot stuffing.

*/

override def newClient(dest: Name, label: String): ServiceFactory[Request, Reply] = {

val quitOnCloseClient = {

new ServiceFactoryProxy[Request, Reply](defaultClient.newClient%28dest, label%29) {

override def apply(conn: ClientConnection): Future[ServiceProxy[Request, Reply]] = {

self.apply(conn) map { service =>

val quitOnClose = new ServiceProxy[Request, Reply](service) {

override def close(deadline: Time): Future[Unit] = {

if (service.isAvailable)

service(Request.Quit)

service.close(deadline)

Wait, I thought you were saying that it wasn't behaving properly in the
dispatcher? I think I got confused as to what was going on . . . I was
trying to reply to a message you posted that said,

Yes, when testing with SMTP server stub and looking at its logs, I found
out that this request is not actually sent from dispatcher, so I sought the
way to enforce service(Request.Quit) before service.close(deadline)

but I couldn't find the actual message (github swallowed it maybe) so I
just put it here.

Is the behavior correct now then?

—
Reply to this email directly or view it on GitHub
https://github.com/twitter/finagle/pull/287/files#r16056704.

С уважением,
Валерия Дымбицкая

right--if you do service(Quit) ensure service.close(deadline) I think that will have the correct behavior, since it will wait for a response before tearing down the connection. the two tcp connections will race to tear down, but I think that's OK.

suncelesta · 2014-08-11T16:20:21Z

Thank you for all the comments! I've made some changes and updated README, so you can check it out.

mosesn · 2014-08-11T16:52:58Z

finagle-smtp/src/main/scala/com/twitter/finagle/smtp/EmailMessage.scala

+    else from.head
+  }
+
+  def to: Seq[MailingAddress] = getAddresses("to")


another way of doing this would be to use an extractor, then you can keep the more natural case style. just a style choice though, it's up to you.

caniszczyk · 2014-08-18T03:49:31Z

@mosesn @travisbrown @selvin now that GSOC is closing up, what can we do to make sure this code gets merged or survives in the github.com/finagle project? Rumor on the street was that @selvin or someone was interested in taking ownership :)

mosesn · 2014-08-18T04:09:10Z

Oh yeah, this LGTM by the way. I'd be happy to see this get merged.

mosesn · 2015-03-03T22:31:31Z

This finally graduated to the finagle organization, and can be found here: https://github.com/finagle/finagle-smtp

suncelesta and others added 25 commits March 13, 2014 00:37

A quick&dirty example of SMTP client

27a48e6

simple wrappers for javamail and a.c.e.

7de8295

Handling only text messages; no tests

New client API

d8f4877

Add simple support for Finagle 6 client API

SMTP commands

3684bf5

Added the ability to send any existing SMTP command + two types of client: one just sending emails and the other sending discrete commands

Reply classes

18f43f2

Dispatcher and transport

728d7f9

+ client dispatcher + transporter + reply decoder

Finished codec

5b9e617

Added handling of server greeting and multiline extension list + command encoder + refactoring of request and reply types

Filters concerning email payload

507a775

+Duplicating leading dot +Adding headers (Not working with copies for now) +Some different factories for EmailMessage

Receiving server greeting before the session

4410eeb

Added connection phase, removed Greeting and handlers constructing it

Reply code refactoring

f5e7604

Minor bugs fix

bed4e94

Commands are dispatched in the correct order Corrected MAIL FROM command + every recipient goes to its own RCPT TO command

More concise result in simple client

699d78d

Got rid of Result in favor of Reply in Smtp and Unit in SmtpSimple (so when simply sending one email, it results either in a Future.Done or a failed Future in case of exception)

Full multiline support

067917c

Extended multiline support on all reply types, removed unused helper classes like ComposedRequest and EmptyReply

Refined structure, tests for filters

14adab8

Added unit tests for filters Created some inner packages, moved Smtp out of package smtp

More tests

69aec96

Tests for SmtpClientDispatcher and ReplyDecoder

Update README.md

8614e7a

Update README.md

017d516

Update README.md

074d72d

Update README.md

a187f29

Update README.md

571e85a

Update README.md

41f35c8

Merge pull request #1 from selvin/master

8626554

fix README to appropriate markdown format and syntax highlighting

Sending quit command upon service closing

f6c8be9

Fixed issue with build error

358655c

suncelesta added 4 commits July 3, 2014 20:08

EHLO sends domain/address literal

adfc22d

Copies and EmailBuilder

cd9efe4

+support for cc, bcc, reply-to +EmailBuilder for composing emails +features for creating MailingAddress and retreiving mailbox string from it

Logs

3ea84a4

Tests for MailAddress and EmailBuilder

53c9b63

mosesn reviewed Aug 6, 2014
View reviewed changes

mosesn reviewed Aug 7, 2014
View reviewed changes

Fixed style and logic

78fe18d

+ Fixed style + EmailBuilder implements EmailMessage + Aggregating multiline requests moved to the dispatcher

mosesn reviewed Aug 8, 2014
View reviewed changes

suncelesta added 2 commits August 11, 2014 19:04

Field names converted to lowercase

a342264

DefaultEmail instead of EmailBuilder

caa31e3

mosesn reviewed Aug 11, 2014
View reviewed changes

suncelesta added 2 commits August 14, 2014 10:47

Quit moved to dispatcher

8adbf4d

Fixes for all tests to pass

03d9be6

mosesn closed this Mar 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSoC: finagle-smtp - initial #287

GSoC: finagle-smtp - initial #287

suncelesta commented Jun 25, 2014

selvin commented Jun 26, 2014

mosesn Aug 6, 2014

mosesn commented Aug 6, 2014

mosesn Aug 7, 2014

suncelesta Aug 8, 2014

mosesn Aug 8, 2014

suncelesta Aug 11, 2014

mosesn Aug 11, 2014

suncelesta Aug 11, 2014

mosesn Aug 11, 2014

suncelesta commented Aug 11, 2014

mosesn Aug 11, 2014

caniszczyk commented Aug 18, 2014

mosesn commented Aug 18, 2014

mosesn commented Mar 3, 2015

GSoC: finagle-smtp - initial #287

GSoC: finagle-smtp - initial #287

Conversation

suncelesta commented Jun 25, 2014

selvin commented Jun 26, 2014

Choose a reason for hiding this comment

mosesn commented Aug 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suncelesta commented Aug 11, 2014

Choose a reason for hiding this comment

caniszczyk commented Aug 18, 2014

mosesn commented Aug 18, 2014

mosesn commented Mar 3, 2015