Add ability to set a client-side timeout on requests #57

sarahkevinking · 2016-04-05T23:30:50Z

If the Ldap process is stopped or otherwise unresponsive,
Bind and SimpleBind will block waiting for a response.

This commit adds BindWithTimeout and SimpleBindWithTimeout to
give the developer control over the wait time. The Bind and
SimpleBind will return an error after DefaultTimeout.

liggitt · 2016-04-05T23:50:58Z

bind.go

+	case res := <-resChan:
+		return res, nil
+	case <-time.After(timeout):
+		return nil, NewError(ErrorNetwork, errors.New("ldap: timeout"))


Does the connection become unusable in this case? What happens if the server responds after this timeout? What if another operation is in progress at that point?

liggitt · 2016-04-06T02:34:39Z

a few general comments

Are bind/simplebind special, or does this behavior apply to all LDAP requests (search, etc)
do we know what happens to the connection once we start working in multiple goroutines? my guess is the connection becomes unusable if we return while we still have a response from a previous request pending

sarahkevinking · 2016-04-06T20:18:41Z

Thank you for the feedback.

This behavior will apply to any LDAP requests that blocks waiting for a response.

Each message has its own out channel that is stored in a channel map conn.chanResults by MessageID. If a message is received close to the timeout it may be possible for a race condition to occur which I will need to address.

Below is an incomplete outline of the changes I am making:

Create a public method Conn.SetTimeout(time.Duration) to define a timeout that will be used for all requests.

Modify processMessages to create a timeout for all requests.

case MessageRequest:
    // Add to message list and write to network
    l.Debug.Printf("Sending message %d", messagePacket.MessageID)
    l.chanResults[messagePacket.MessageID] = messagePacket.Channel
    go func(messageID int64) {
        _, ok := <- time.After(l.timeout)
        if ok {
            message := &messagePacket{
                Op:        MessageTimeout,
                MessageID: messageID,
            }
            l.sendProcessMessage()
        }
    }(messagePacket.messageID)
    // go routine
    buf := messagePacket.Packet.Bytes()

    _, err := l.conn.Write(buf)
    if err != nil {
        l.Debug.Printf("Error Sending Message: %s", err.Error())
        break
    }

Modify MessageFinish to close the timeout channel to stop the timeout from writing to the message channel.

Remove the BindWithTimeout and SimpleBindWithTimeout methods.

liggitt · 2016-04-07T20:07:19Z

bind.go

-	if packet == nil {
-		return nil, NewError(ErrorNetwork, errors.New("ldap: could not retrieve response"))
+	packetResponse, ok := <-channel
+	packet, err = packetResponse.ReadPacket()


this will panic if packetResponse is nil

I take it back, hadn't gotten to the nil receiver impl below... that's nonstandard enough that I think you should always check ok before doing anything with packetResponse

alternately, don't check ok at all and trust the nil receiver impl of PacketResponse

liggitt · 2016-04-07T21:51:33Z

looking really good. can you update the interface in client.go, and is there a way to add a test for this?

sarahkevinking · 2016-04-07T22:06:55Z

I'm controlling my own ldap process in our go tests, but I could test it by creating an ldap.Conn, sending a message, and asserting that the timeout is triggered. Would that suffice?

liggitt · 2016-04-08T05:39:05Z

conn_test.go

+	if err.Error() != "ldap: connection timed out" {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	conn.Close()


already deferred this above

liggitt · 2016-04-08T05:43:28Z

Test looks good, thanks. You can go ahead and squash to a single commit.

Add SetTimeout to Client API to configure timeout Add MessageTimeout event handler Return explicit errors from message handler

liggitt · 2016-04-10T04:14:52Z

bind.go

+	if err != nil {
+		return nil, err
+	}
+	if !ok {


check this ok before doing anything with packetResponse

liggitt · 2016-04-10T04:18:06Z

since PacketResponse#ReadPacket() that returns an error if the receiver is nil, I'm not sure we need to check for a closed channel in all those places. I think I'd rather just do this:

packetResponse := <-channel
packet, err = packetResponse.ReadPacket()
if err != nil {
  ...
}
...

sarahkevinking · 2016-04-12T20:57:26Z

@liggitt I went ahead and made the change that you asked for. Let me know if there is anything else that you notice before it can be merged in.

liggitt · 2016-04-15T03:58:40Z

thanks, this looks good

jamesboswell · 2016-04-24T22:05:43Z

@liggitt can this be tagged for gopkg.in ? Being able to set client side timeout would be very useful.

liggitt · 2016-04-25T01:17:45Z

Yeah, I was just trying to decide if it was a point release or major version (since changing an interface or adding a public method can break some consumers)

liggitt · 2016-04-25T01:19:04Z

And if it was a major version, if there were any other changes we wanted to include while were at it

liggitt · 2016-05-02T17:35:37Z

tagged in v2.3.0

* add ModifyDN() - moddn/modrdn * Update README * add ModifyDN() to client * update docs, changes needed for go-ldap#57 * update to use msgCtx * fix missing comma, add moddn_test.go with examples * rename example functions to match the conventions for godoc * add missing parens

Update README update docs, changes needed for #57 add ModifyDN() to client update to use msgCtx fix missing comma, add moddn_test.go with examples rename example functions to match the conventions for godoc add missing parens

liggitt reviewed Apr 5, 2016
View reviewed changes

liggitt reviewed Apr 7, 2016
View reviewed changes

liggitt reviewed Apr 8, 2016
View reviewed changes

sarahkevinking force-pushed the master branch 5 times, most recently from 6d4e0a8 to 01788e2 Compare April 8, 2016 20:16

Timeout unresponsive ldap connections

01788e2

Add SetTimeout to Client API to configure timeout Add MessageTimeout event handler Return explicit errors from message handler

liggitt reviewed Apr 10, 2016
View reviewed changes

bind.go

if err != nil {

return nil, err

}

if !ok {

Copy link

Contributor

liggitt Apr 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check this ok before doing anything with packetResponse

Clean up packet error handling

cf1b293

trestletech mentioned this pull request Apr 11, 2016

nil pointer panic when StartTLS fails #58

Closed

liggitt merged commit 0e7db8e into go-ldap:master Apr 15, 2016

liggitt changed the title ~~Apply DefaultTimeout to Bind and SimpleBind~~ Add ability to set a client-side timeout on requests Apr 15, 2016

jlhawn mentioned this pull request May 19, 2016

Improve handling of connection read errors #66

Merged

liggitt mentioned this pull request Jun 8, 2016

add ModifyDN() - moddn/modrdn #54

Closed

vetinari added a commit to vetinari/ldap that referenced this pull request Jun 9, 2016

update docs, changes needed for go-ldap#57

81dd048

jamesboswell pushed a commit to jamesboswell/ldap that referenced this pull request Dec 21, 2017

update docs, changes needed for go-ldap#57

495943b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set a client-side timeout on requests #57

Add ability to set a client-side timeout on requests #57

sarahkevinking commented Apr 5, 2016

liggitt Apr 5, 2016

liggitt commented Apr 6, 2016

sarahkevinking commented Apr 6, 2016

liggitt Apr 7, 2016

liggitt Apr 7, 2016

liggitt Apr 7, 2016

liggitt commented Apr 7, 2016

sarahkevinking commented Apr 7, 2016

liggitt Apr 8, 2016

liggitt commented Apr 8, 2016

liggitt Apr 10, 2016

liggitt commented Apr 10, 2016

sarahkevinking commented Apr 12, 2016

liggitt commented Apr 15, 2016

jamesboswell commented Apr 24, 2016 •

edited

Loading

liggitt commented Apr 25, 2016

liggitt commented Apr 25, 2016

liggitt commented May 2, 2016

Add ability to set a client-side timeout on requests #57

Add ability to set a client-side timeout on requests #57

Conversation

sarahkevinking commented Apr 5, 2016

liggitt Apr 5, 2016

Choose a reason for hiding this comment

liggitt commented Apr 6, 2016

sarahkevinking commented Apr 6, 2016

liggitt Apr 7, 2016

Choose a reason for hiding this comment

liggitt Apr 7, 2016

Choose a reason for hiding this comment

liggitt Apr 7, 2016

Choose a reason for hiding this comment

liggitt commented Apr 7, 2016

sarahkevinking commented Apr 7, 2016

liggitt Apr 8, 2016

Choose a reason for hiding this comment

liggitt commented Apr 8, 2016

liggitt Apr 10, 2016

Choose a reason for hiding this comment

liggitt commented Apr 10, 2016

sarahkevinking commented Apr 12, 2016

liggitt commented Apr 15, 2016

jamesboswell commented Apr 24, 2016 • edited Loading

liggitt commented Apr 25, 2016

liggitt commented Apr 25, 2016

liggitt commented May 2, 2016

jamesboswell commented Apr 24, 2016 •

edited

Loading