Skip to content
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

GSoC: finagle-smtp - initial #287

Closed
wants to merge 50 commits into from

Conversation

@suncelesta
Copy link

commented Jun 25, 2014

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.

suncelesta and others added some commits Mar 12, 2014

simple wrappers for javamail and a.c.e.
Handling only text messages; no tests
New client API
Add simple support for Finagle 6 client API
SMTP commands
Added the ability to send any existing SMTP command + two types of
client: one just sending emails and the other sending discrete commands
Dispatcher and transport
+ client dispatcher
+ transporter
+ reply decoder
Finished codec
Added handling of server greeting and multiline extension list
+ command encoder
+ refactoring of request and reply types
Filters concerning email payload
+Duplicating leading dot
+Adding headers
(Not working with copies for now)
+Some different factories for EmailMessage
Receiving server greeting before the session
Added connection phase, removed Greeting and handlers constructing it
Minor bugs fix
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
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
Extended multiline support on all reply types, removed unused helper
classes like ComposedRequest and EmptyReply
Refined structure, tests for filters
Added unit tests for filters
Created some inner packages, moved Smtp out of package smtp
More tests
Tests for SmtpClientDispatcher and ReplyDecoder
Multiline correction and usage doc
+ 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
Merge pull request #1 from selvin/master
fix README to appropriate markdown format and syntax highlighting
@selvin

This comment has been minimized.

Copy link

commented Jun 26, 2014

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?

suncelesta added some commits Jul 3, 2014

Copies and EmailBuilder
+support for cc, bcc, reply-to
+EmailBuilder for composing emails
+features for creating MailingAddress and retreiving mailbox string from
it
val second = rep(1)
val third = rep(2)

//Standart reply: three-digit-code SP info

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Standart => Standard

class ReplyDecoder extends LineBasedFrameDecoder(1000) {
import CodecUtil._
override def decode(ctx: ChannelHandlerContext, channel: Channel, msg: ChannelBuffer): UnspecifiedReply = {
val buf = super.decode(ctx, channel, msg)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

wrap this in an Option and we can just match on it.

val buf = super.decode(ctx, channel, msg)
if (buf == null) null
else buf match {
case cb: ChannelBuffer => {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

what other type can this be?

else buf match {
case cb: ChannelBuffer => {
val rep = cb.toString(CharsetUtil.UTF_8)
val first = rep(0)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

we can use destructuring to do this more concisely. do we want to do some validation here to avoid an array out of bounds issue?

* Encodes a Request into a ChannelBuffer.
*/
class SmtpEncoder extends SimpleChannelDownstreamHandler {
override def writeRequested(ctx: ChannelHandlerContext, evt: MessageEvent) =

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add return type annotation

evt.getMessage match {
case req: Request =>
try {
val buf = ChannelBuffers.copiedBuffer(req.cmd + "\r\n", CharsetUtil.US_ASCII)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

why is this ascii?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

Because the default charset for SMTP is 7-bit ASCII. This version doesn't implement any extensions dealing with 8-bit and binary data, it is to be in the next one.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

OK, I got confused because you use utf8 elsewhere in the codebase. I commented on the bit where you use utf8.

@@ -0,0 +1,162 @@
package com.twitter.finagle.smtp

import org.junit.runner.RunWith

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

sort lines

import org.junit.runner.RunWith
import org.scalatest.junit.JUnitRunner
import org.scalatest.FunSuite
import com.twitter.util.Try

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

sort lines


import org.junit.runner.RunWith
import org.scalatest.junit.JUnitRunner
import org.scalatest.FunSuite

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

sort imports


test("mailboxList") {
val list = Seq(
MailingAddress("1@ex.com"),

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

indent these by two spaces.

import org.specs.mock.MockitoStubs

@RunWith(classOf[JUnitRunner])
class ReplyDecoderTest extends FunSuite with MockitoStubs {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

let's use MockitoSugar (from scalatest) instead of MockitoStubs (from specs).

sharedSettings
).settings(
name := "finagle-smtp",
libraryDependencies ++= Seq(util("logging")/*,

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

why is this commented out?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

I thought about making a wrapper for this library in the beginning but decided not to do that yet, so I commented this out and forgot to clean, sorry.

@mosesn

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2014

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. 👍

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

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

should this be ascii?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 8, 2014

Author

Yes, I must have missed that one, thanks.

Fixed style and logic
+ Fixed style
+ EmailBuilder implements EmailMessage
+ Aggregating multiline requests moved to the dispatcher
override def close(deadline: Time): Future[Unit] = {
if (service.isAvailable)
service(Request.Quit)
service.close(deadline)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 8, 2014

Contributor

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?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 11, 2014

Author

According to wireshark:

  1. QUIT is sent
  2. Server responds
  3. Server closes connection
  4. Client closes connection

Seems like what should be expected.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

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?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 11, 2014

Author

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 notifications@github.com:

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.

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

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

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.

/**
* Constructs an [[com.twitter.finagle.smtp.EmailMessage]].
*/
case class EmailBuilder(

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

thinking about java compatibility, default arguments make this really hard to use from java. I think it's fine to keep these, but could you make a couple other nice builder methods that can be used from java? Maybe something that doesn't take any arguments.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

I'm not sure EmailBuilder is the right name for this anymore, since we never call build on it. How about DefaultEmail?

*
* @param addrs Addresses to be added
*/
def from_(addrs: String*): EmailBuilder = setFrom(from ++ addrs.map(MailingAddress(_)))

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

where does this trailing underscore syntax come from? is this an smtp thing? why does this take varargs String where setFrom takes Seq[String]?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 11, 2014

Author

It is just to not confuse with methods from EmailMessage, but to allow composing email fields naturally, like to_(addr1, addr2).
As for varargs - I supposed that methods like setFrom should be used more like a way to prepend or remove addresses, so it didn't make much sense for them to take varargs.

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 11, 2014

Author

I found out that I also forgot to change Example this way, I'll fix that.

@suncelesta

This comment has been minimized.

Copy link
Author

commented Aug 11, 2014

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

else from.head
}

def to: Seq[MailingAddress] = getAddresses("to")

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

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.

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

val quitOnCloseClient = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 11, 2014

Contributor

thinking about this more, I think this is incorrect. we don't want to close the connection when we close this service, we just want to relinquish the connection so that it can be used elsewhere. this should definitely be moved into the dispatcher.

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 12, 2014

Author

Ok, I've tried to use wireshark with QUIT sent in dispatcher, and it shows that the request is indeed not sent. The connection is just closed without even attempting to send it. Maybe I'm just doing it wrong? It appeared to me that I should override close(deadline: Time) method - is it right?

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 12, 2014

Contributor

Have you tried what I suggested about service(Quit) ensure { service.close }? Did that have the same behavior?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 12, 2014

Author

It's what I've recently pushed (look below), and it has correct behavior.

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 12, 2014

Author

And in dispatcher the behavior is still incorrect when using ensure.

Future.Done
} ensure {
service.close(deadline)
}

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 12, 2014

Author

Here is what you suggested, I believe. Correct me if I misunderstood, I may have lost track of thought here.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 12, 2014

Contributor

Sorry, I'm having trouble reasoning about the dispatcher code without seeing it. Could you link me to a branch which has the quit in the dispatcher?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 13, 2014

Author

Ok, I've created a new branch. Here are the changes for you to review.

suncelesta added some commits Aug 14, 2014

@caniszczyk

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2014

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2014

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

@mosesn

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2015

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

@mosesn mosesn closed this Mar 3, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
6 participants
You can’t perform that action at this time.