Grokbase › Groups › Go › golang-dev › December 2012
FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
net: undo part of CL6ec24fe2e501.

The removal of fd.Close might leave a netFD on an
invalid Handle with a non-nil finalizer on Windows
systems.

Please review this at https://codereview.appspot.com/6847129/

Affected files:
M src/pkg/net/sock_posix.go


Index: src/pkg/net/sock_posix.go
===================================================================
--- a/src/pkg/net/sock_posix.go
+++ b/src/pkg/net/sock_posix.go
@@ -61,6 +61,7 @@
}
if err = fd.connect(ursa); err != nil {
closesocket(s)
+ fd.Close()
return nil, err
}
fd.isConnected = true

Search Discussions

  • Dave Cheney at Dec 1, 2012 at 12:53 pm ⇧
    LGTM.
    On Sat, Dec 1, 2012 at 11:49 PM, wrote:
    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

    I'd like you to review this change to
    https://go.googlecode.com/hg/


    Description:
    net: undo part of CL6ec24fe2e501.

    The removal of fd.Close might leave a netFD on an
    invalid Handle with a non-nil finalizer on Windows
    systems.

    Please review this at https://codereview.appspot.com/6847129/

    Affected files:
    M src/pkg/net/sock_posix.go


    Index: src/pkg/net/sock_posix.go
    ===================================================================
    --- a/src/pkg/net/sock_posix.go
    +++ b/src/pkg/net/sock_posix.go
    @@ -61,6 +61,7 @@
    }
    if err = fd.connect(ursa); err != nil {
    closesocket(s)
    + fd.Close()
    return nil, err
    }
    fd.isConnected = true
  • Mikioh Mikioh at Dec 1, 2012 at 1:26 pm ⇧
    LGTM

    https://codereview.appspot.com/6847129/
  • Brad Fitzpatrick at Dec 1, 2012 at 4:54 pm ⇧
    I haven't been following this closely, but it seems subtle enough if
    several of you have been confused about it in the original CL and now,
    having to revert part of it.

    It also seems testable.

    At the least, you should probably wait for Alex or whoever wrote the
    Windows net stuff.
    On Sat, Dec 1, 2012 at 4:49 AM, wrote:

    Reviewers: golang-dev_googlegroups.com,

    Message:
    Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

    I'd like you to review this change to
    https://go.googlecode.com/hg/


    Description:
    net: undo part of CL6ec24fe2e501.

    The removal of fd.Close might leave a netFD on an
    invalid Handle with a non-nil finalizer on Windows
    systems.

    Please review this at https://codereview.appspot.**com/6847129/<https://codereview.appspot.com/6847129/>

    Affected files:
    M src/pkg/net/sock_posix.go


    Index: src/pkg/net/sock_posix.go
    ==============================**==============================**=======
    --- a/src/pkg/net/sock_posix.go
    +++ b/src/pkg/net/sock_posix.go
    @@ -61,6 +61,7 @@
    }
    if err = fd.connect(ursa); err != nil {
    closesocket(s)
    + fd.Close()
    return nil, err
    }
    fd.isConnected = true

  • Alex Brainman at Dec 1, 2012 at 11:41 pm ⇧
    I do not know what to do.

    I do not follow net changes closely. There are too many.

    I also looked in the history, and it does not help.

    scok_posix.go is shared by many os. Therefore, whatever goes in there
    should be sensible for every os, not just for some. Is Close needed in
    here for unix? If not, perhaps, windows code need to be rearranged
    accordingly.

    Feel free to do whatever you think is right - submit or drop this CL. Do
    not worry about braking windows build. I will investigate this anyway
    when I have more time.

    Alex

    https://codereview.appspot.com/6847129/
  • Mikio Hara at Dec 2, 2012 at 2:19 am ⇧
    FYI

    issue: https://code.google.com/p/go/issues/detail?id=1884
    changeset: 8965:dc01810c2648
  • Brad Fitzpatrick at Dec 2, 2012 at 2:24 am ⇧
    wrong issue? SPDY?
    On Sat, Dec 1, 2012 at 6:19 PM, Mikio Hara wrote:

    FYI

    issue: https://code.google.com/p/go/issues/detail?id=1884
    changeset: 8965:dc01810c2648
  • Mikio Hara at Dec 2, 2012 at 2:25 am ⇧
    oops.
    issue: https://code.google.com/p/go/issues/detail?id=1884
    correct issue: https://code.google.com/p/go/issues/detail?id=1487
  • Mikio Hara at Dec 2, 2012 at 4:48 am ⇧
    recap.

    - issue: https://code.google.com/p/go/issues/detail?id=1487
    changeset: 8965:dc01810c2648

    replaced closesocket with fd.Close for Windows port, then,

    - issue: https://code.google.com/p/go/issues/detail?id=2349
    changeset: 9995:7d78fe7f328e

    fixed active open socket stuck in SYN_SENT queue in protocol stack
    when connect syscall failed.
  • Dave Cheney at Dec 2, 2012 at 9:21 am ⇧
    Nice detective work Mikio. I can see how we ended up with the current logic.
    On Sun, Dec 2, 2012 at 3:48 PM, Mikio Hara wrote:
    recap.

    - issue: https://code.google.com/p/go/issues/detail?id=1487
    changeset: 8965:dc01810c2648

    replaced closesocket with fd.Close for Windows port, then,

    - issue: https://code.google.com/p/go/issues/detail?id=2349
    changeset: 9995:7d78fe7f328e

    fixed active open socket stuck in SYN_SENT queue in protocol stack
    when connect syscall failed.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedDec 1, '12 at 12:49p
activeDec 2, '12 at 9:21a
posts10
users5
websitegolang.org

5 users in discussion

Mikio Hara: 4 posts Dave Cheney: 2 posts Brad Fitzpatrick: 2 posts Alex Brainman: 1 post Remyoudompheng: 1 post

Content

People

Support

Translate

site design / logo © 2022 Grokbase