Use Message Context

[jlhawn]

This patch introduces a new type called messageContext which is now the
return value of (*Conn).sendMessage(). The message context object still
contains a channel from which methods like, Add(), Bind(), Search(), etc.,
will receive response packets. It also has a field which holds the message
ID as well as a done channel which is used to prevent deadlock in the
processMessages() goroutine.

This is accomplished by also changing the (*Conn).finishMessage() method to
take a message context and close this done channel before sending a
MessageFinish packet to the processMessages() goroutine.

The processMessages() goroutine now has a messageContexts map which
replaces the chanResults map. Now, rather than sending response packets only
on the response channels, the messageContext has its own sendResponse()
method which uses a switch that blocks on sending a response packet or
waiting for its done channel to be closed by finishMessage().

fixes #68

[jlhawn]

@liggitt PTAL

[liggitt]

Will look, thanks!

[liggitt]

shouldn't this happen before the the isClosing check? otherwise we may still have potential for deadlocks

[liggitt]

looks good, just a few comments. can you add a test that tries to trigger this as well (simulated request that returns multiple response packets when only one is expected)? ideally, it would fail without this patch in place (though the test might race with the finishRequest() message)

[jlhawn]

@liggitt Thanks for the quick review, I have updated the PR per your comments and added a new TestConn test case which fails on master but passes on this branch. I couldn't find any other tests that seem to simulate an LDAP server so I created a new type which I think you'll find very useful for other tests:

// packetTranslatorConn is a helful type which can be used with various tests
// in this package. It implements the net.Conn interface to be used as an
// underlying connection for a *ldap.Conn. Most methods are no-ops but the
// Read() and Write() methods are able to translate ber-encoded packets for
// testing LDAP requests and responses.
//
// Test cases can simulate an LDAP server sending a response by calling the
// SendResponse() method with a ber-encoded LDAP response packet. Test cases
// can simulate an LDAP server receiving a request from a client by calling the
// ReceiveRequest() method which returns a ber-encoded LDAP request packet.
type packetTranslatorConn struct {
    sync.Mutex
    isClosed bool

    responseCond sync.Cond
    requestCond  sync.Cond

    responseBuf bytes.Buffer
    requestBuf  bytes.Buffer
}

You can use it in tests like this:

func TestSomething(t *testing.T) {
    ptc := newPacketTranslatorConn()
    defer ptc.Close()

    conn := NewConn(ptc, false)
    conn.Start()
    defer conn.Close()

    // Create an LDAP request packet and send it as usual.
    requestPacket := makeRequestPacket()
    msgCtx, _ := conn.sendMessage()

    // Decode the request packet on the other side of the connection.
    serverRequest, _ := ptc.ReceiveRequest()

    // Create a response packet and send it.
    serverResponse := makeResponsePacket()
    ptc.SendResponse(serverResponse)

    // Handle the response as usual.
    responsePacket <- msgCtx.responses
}

[liggitt]

millisecond timing is way too tight for something depending on a loop inside a separate goroutine... can lead to test flake failures if anything else is hitting the CPU while the test is running. If the main purpose is to prevent a test hanging forever, I'd do something like a second

[jlhawn]

@liggitt thanks for the comments. I've updated the branch again.

added a new TestConn test case which fails on master but passes on this branch.

I forgot to mention that the new test doesn't build on master without a few modifications.

[jlhawn]

The best way to see the new tests fail is to just comment out this line.

[liggitt]

thanks for the report and fix