code review 10082044: net/smtp: close conn in SendMail; add Client.Close method

net/smtp: close conn in SendMail; add Client.Close method

[toalexjin, 2013-06-07 14:40:52]

I am new to Go language development. Please help to review my first checkin,
thanks a lot!

[rsc, 2013-06-10 20:38:21]

Please run 'hg mail 10082044' to start a review.

[rsc, 2013-06-10 20:38:47]

Also please complete a CLA as described at
golang.org/doc/contribute.html#copyright

[toalexjin, 2013-06-13 02:21:45]

Hello rsc@golang.org, golang-dev@googlegroups.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[toalexjin, 2013-06-13 02:28:04]

rsc,

Thanks for your feedback. Based on your instructions, I signed Individual
Contributor License Agreement online just now, and also ran "hg mail
10082044". Please review my checkin (10082044), thanks a lot.


Alex Jin.



2013/6/11 <rsc@golang.org>

> Also please complete a CLA as described at
>
golang.org/doc/contribute.**html#copyright<http://golang.org/doc/contribute.html#copyright>
>
>
>
https://codereview.appspot.**com/10082044/<https://codereview.appspot.com/100...
>

[bradfitz, 2013-06-14 18:02:11]

https://codereview.appspot.com/10082044/diff/10001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/10082044/diff/10001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:268: c, err := NewClient(conn,
addr[:strings.Index(addr, ":")])
This doesn't work correctly for an IPv6 literal + port.

Use net.SplitHostPort instead.

[toalexjin, 2013-06-15 16:20:41]

Hello rsc@golang.org, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-06-17 14:50:54]

https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:43: // Dial returns a new Client connected to an SMTP
server at addr.
Not your fault, but I realize that this comment doesn't clarify that an addr is
required.  Another plausible way to read this sentence is that it connects to
port 25 by default, since it does say "an SMTP server at".

I don't think we should make 25 the default, but the sentence should become
clear.

Then the error checking on SplitHostPort can go away, since net.Dial will do
that anyway.  Then you can just say host, _, _ := net.SplitHostPort(addr) after
the net.Dial line.

https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:275: defer c.Close()
if this were defer c.Text.Close(), then you can get rid of the newly-added
Client.Close method above, which is an API addition but not noted in your CL
description.

[toalexjin, 2013-06-17 16:47:19]

Hello rsc@golang.org, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-06-17 16:51:55]

Update the CL to be more succinct:

net/smtp: close conn in SendMail; add Client.Close method

https://codereview.appspot.com/10082044/diff/28001/src/pkg/net/smtp/smtp.go
File src/pkg/net/smtp/smtp.go (right):

https://codereview.appspot.com/10082044/diff/28001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:44: //
Just:

// Dial returns a new Client connected to an SMTP server at addr.
// The addr must include a port number.

Then delete the rest.

https://codereview.appspot.com/10082044/diff/28001/src/pkg/net/smtp/smtp.go#n...
src/pkg/net/smtp/smtp.go:77: // Close closes the connection.
Just keep this line and delete the rest.  We don't need to teach people how to
write Go code in a comment on a Close method.

[toalexjin, 2013-06-17 16:53:57]

On 2013/06/17 14:50:54, bradfitz wrote:
> https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go
> File src/pkg/net/smtp/smtp.go (right):
> 
>
https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go#n...
> src/pkg/net/smtp/smtp.go:43: // Dial returns a new Client connected to an SMTP
> server at addr.
> Not your fault, but I realize that this comment doesn't clarify that an addr
is
> required.  Another plausible way to read this sentence is that it connects to
> port 25 by default, since it does say "an SMTP server at".
> 
> I don't think we should make 25 the default, but the sentence should become
> clear.

  [Alex Jin] You are right, I added more comments to function Dial().

> Then the error checking on SplitHostPort can go away, since net.Dial will do
> that anyway.  Then you can just say host, _, _ := net.SplitHostPort(addr)
after
> the net.Dial line.

  [Alex Jin] I made change as you suggested.
 
>
https://codereview.appspot.com/10082044/diff/20001/src/pkg/net/smtp/smtp.go#n...
> src/pkg/net/smtp/smtp.go:275: defer c.Close()
> if this were defer c.Text.Close(), then you can get rid of the newly-added
> Client.Close method above, which is an API addition but not noted in your CL
> description.
  [Alex Jin] Come back to think about this part, I think Close() is still
required because
             we do not want the caller/end user to know more details about the
Client object.
             Offering function Close() makes the module more clear to use. Same
as Dial(),
             I added some comments to Close(). Besides, I modified CL
description to include this change.
 
Please see my comments below, and review my code again, thanks!

[toalexjin, 2013-06-17 17:02:40]

Hello rsc@golang.org, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-06-17 17:07:33]

LGTM

[bradfitz, 2013-06-17 23:53:32]

*** Submitted as https://code.google.com/p/go/source/detail?r=7961b83d1d37 ***

net/smtp: close conn in SendMail; add Client.Close method

R=rsc, dave, bradfitz
CC=golang-dev
https://codereview.appspot.com/10082044

Committer: Brad Fitzpatrick <bradfitz@golang.org>