Add search asynchronously with context #440

t2y · 2023-06-01T04:59:45Z

After I discussed with @cpuschma and @johnweldon, we decided to provide a search asynchronous function.

SearchAsync(ctx context.Context, searchRequest *SearchRequest, bufferSize int) Response

type Response interface {
	Entry() *Entry
	Referral() string
	Controls() []Control
	Err() error
	Next() bool
}

This PR is inspired by #319, I appreciate @stefanmcshane.

#319 is a draft, and it seems there is no activity now. So, I recreated the functionality with the addition of my idea.

This PR is different from #319. I describe them below.

manage channel and goroutine in ldap.Conn since a developer can use it easily
can cancel with context by the caller's side
add SearchSingleResult for a single entry and handle an error

func (l *Conn) SearchWithChannel(ctx context.Context, searchRequest *SearchRequest) chan *SearchSingleResult

// SearchSingleResult holds the server's single response to a search request
type SearchSingleResult struct {
	// Entry is the returned entry
	Entry *Entry
	// Referral is the returned referral
	Referral string
	// Controls are the returned controls
	Controls []Control
	// Error is set when the search request was failed
	Error error
}

I'm new to LDAP protocol, so any comments are welcome.

References

cpuschma

Thank you for refreshing the PR.

I would like to question the design for asynchronous search queries in general: What do you think about a design similar to go/databases/sql/driver.go, like:

result, err := conn.SearchAsync(...)
if err != nil {
    // ...
}

for result.Next() {
    entry := result.Entry()
}

Thus one would not have to release a channel directly and would be freer in the implementation which goes to future changes.

@stefanmcshane @vetinari @johnweldon I would be glad about your opinions

search.go

cpuschma · 2023-06-03T11:56:43Z

search.go

+// e.g. for size / time limited requests all are recieved via the channel
+// until the limit is reached.
+func (l *Conn) SearchWithChannel(ctx context.Context, searchRequest *SearchRequest) chan *SearchSingleResult {
+	ch := make(chan *SearchSingleResult)


I am conflicted about the channels, already with the first pull request. I would like to leave as many options open to the developer, such as determining the channel size. A suggestion and I would ask for general feedback here:

An additional argument for the function channelSize. If channelSize is 0, a normal channel without a certain size is created like now. If the value is greater than 0, a buffered channel with the defined size is created, e.g.

var chan *SearchSingleResult if channelSize > 0 { chan = make(chan *SearchSingleResult, channelSize) } else { chan = make(chan *SearchSingleResult) }

Make sense. Also, SearchWithPaging takes pagingSize, so it seems it's no wonder API SearchWithChannel takes channelSize.

func (l *Conn) SearchWithPaging(searchRequest *SearchRequest, pagingSize uint32) (*SearchResult, error) {

I confirmed a deadlock issue using a channel with zero buffer size sometimes. Asynchronous is difficult.

I guess it occurs running with the below order.

caller

1: result <- ch

4: cancel() // never cancel when the search is blocked to write the next result

5: result <- ch // block

search

2: ch <- result

3: ch <- result // block

I guess another deadlock issue with zero buffer size is here: https://github.com/go-ldap/ldap/actions/runs/5167123452/jobs/9307800176?pr=440

client

3 defer l.Close() //block

search

1: ch <- result
2: ch <- result // block

To unlock the mutex, defer l.finishMessage(msgCtx) must be called. Got it.

I think this is not a bug on go-ldap code. So I fixed on caller's code.

cpuschma · 2023-06-03T12:04:40Z

search.go

+				packetResponse, ok := <-msgCtx.responses
+				if !ok {
+					err := NewError(ErrorNetwork, errors.New("ldap: response channel closed"))
+					ch <- &SearchSingleResult{Error: err}


These writes do not account for a closed channel, which would cause a panic. Since this goroutines does not have a recover in the deferred function, the program would crash.

If we stay with this design, I suggest to add a warning to the function's description to not close the channel outside the library.

Good review! I changed the returned channel to receive-only. By doing this, the compiler rejects when the caller closes it.

$ go run main.go # command-line-arguments ./main.go:62:10: invalid operation: cannot close receive-only channel ch (variable of type <-chan *ldap.SearchSingleResult)

In any case, I learned recover is needed to avoid panic from https://github.com/go-ldap/ldap/actions/runs/5166940415/jobs/9307492985.

… caller go-ldap#319

… the caller go-ldap#319

t2y · 2023-06-04T03:21:28Z

I agree with conn.SearchAsync(...) API design when we provide a more robust and extendable API. I will change the API signature when other reviewers want.

…connection go-ldap#319

t2y · 2023-06-04T05:24:19Z

I found conn.SearchAsync(...) API also suggested on #341. So I can merge some requirements if they are needed.

t2y · 2023-06-05T02:54:01Z

@cpuschma I implemented asynchronous search for v3 only (a9daeeb) as a basis for discussion. I hope it's helpful to discuss the design we require.

t2y · 2023-06-10T00:33:57Z

How do we get a consensus asynchronous search design to move this PR forward?

johnweldon · 2023-06-12T18:56:47Z

Thanks for the updates @t2y

I'm also skeptical of exposing an internally created channel on the public API of the library.

I'd like feedback from others too though @go-ldap/committers

t2y · 2023-06-13T00:22:38Z

@johnweldon Thank you for reviewing. I also added conn.SearchAsync(...) API as a reference implementation. Tell me if you have a good idea.

SearchAsync(ctx context.Context, searchRequest *SearchRequest, bufferSize int) Response

type Response interface {
	Entry() *Entry
	Referral() string
	Controls() []Control
	Err() error
	Next() bool
}

cpuschma · 2023-06-15T17:22:24Z

As stated in my review, I would go for the design similar to the sql.Rows interface of the official sql library. It would allow us to keep the internals, which makes changes easier. I think an interface like t2ys one is sufficient

type Response interface {
	Entry() *Entry
	Referral() string
	Controls() []Control
	Err() error
	Next() bool
}

t2y · 2023-06-16T00:45:37Z

We have three options now.

provides only SearchWithChannel() returns the channel
provides only SearchAsync() returns the abstract Response interface
provides both SearchWithChannel() and SearchAsync()

At the current moment, we're considering No.2 (only SearchAsync()) as better, right?

johnweldon · 2023-06-19T17:53:56Z

Agreed; I vote no.2 as the preferred choice.

t2y · 2023-06-19T22:56:46Z

@cpuschma @stefanmcshane @vetinari What do you think above three options? I think No. 2 is better, too.

cpuschma · 2023-06-21T18:15:13Z

Sorry I'm currently sick. I would also go for Option 2 with the SQL like syntax

t2y · 2023-06-22T00:16:58Z

Thanks for the comments. I'm going to update the implementation to provide No.2 only.

…ldap#319 go-ldap#341

t2y · 2023-06-22T01:08:09Z

Got race condition with go 1.16.x.

  github.com/go-ldap/ldap.(*Conn).GetLastError()
      /home/runner/work/ldap/ldap/conn.go:330 +0x204

--- FAIL: TestSearchAsyncAndCancel (0.06s)
    ldap_test.go:411: TestSearchAsyncAndCancel: (&(objectclass=rfc822mailgroup)(cn=*Computer*)) -> num of entries = 11
    testing.go:1102: race detected during execution of test

https://github.com/go-ldap/ldap/actions/runs/5340457447/jobs/9680294821

…cation

t2y · 2023-06-22T01:25:44Z

@cpuschma @johnweldon @stefanmcshane @vetinari I cleaned up this PR to provide SearchAsync() only. Could you review it?

cpuschma · 2023-06-28T14:14:22Z

I did check the source during lunchtime. LGTM so far, I'll run a few tests later and give you feedback as soon as I'm finished

johnweldon

LGTM

Thanks

cpuschma · 2023-06-30T21:16:33Z

I'll prepare a pre-release for people to test the new feature

…is trying to lock, and another goroutine waits since full of channel buffers go-ldap#440

…is trying to lock, and another goroutine waits since full of channel buffers #440 (#446)

feat: add search with channels inspired by go-ldap#319

81ab75d

t2y mentioned this pull request Jun 1, 2023

Added search with channels first draft #319

Closed

cpuschma added the enhancement label Jun 3, 2023

cpuschma requested changes Jun 3, 2023

View reviewed changes

cpuschma added the help wanted label Jun 3, 2023

t2y added 4 commits June 4, 2023 11:33

refactor: fix to check proper test results go-ldap#319

1131fd5

refactor: fix to use unpackAttributes() for Attributes go-ldap#319

60bc080

refactor: returns receive-only channel to prevent closing it from the…

03e1d76

… caller go-ldap#319

refactor: pass channelSize to be able to controll buffered channel by…

51a1a4f

… the caller go-ldap#319

t2y added 2 commits June 4, 2023 13:01

fix: recover an asynchronouse closing timing issue go-ldap#319

698ccfe

fix: consume all entries from the channel to prevent blocking by the …

7279710

…connection go-ldap#319

feat: add initial search async function with channel go-ldap#341

a9daeeb

feat: provide search async function and drop search with channels go-…

2f623f0

…ldap#319 go-ldap#341

refactor: lock when to call GetLastError since it might be in communi…

9de41ce

…cation

t2y changed the title ~~Add search with channels with context~~ Add search asynchronously with context Jun 22, 2023

johnweldon approved these changes Jun 28, 2023

View reviewed changes

cpuschma approved these changes Jun 30, 2023

View reviewed changes

cpuschma merged commit 7778a1c into go-ldap:master Jun 30, 2023
16 checks passed

This was referenced Jul 2, 2023

About implementing syncrepl (rfc-4533) consumer #422

Closed

ldap notifications: add SearchAsync #341

Closed

t2y deleted the search-with-channel branch July 3, 2023 13:46

t2y added a commit to t2y/ldap that referenced this pull request Jul 19, 2023

fix: fix a deadlock issue when it occurs simultaneously: a goroutine …

741471d

…is trying to lock, and another goroutine waits since full of channel buffers go-ldap#440

This was referenced Jul 19, 2023

Fix a deadlock issue using search asynchronously #446

Merged

Add syncrepl (rfc-4533) consumer (persistent search) #447

Merged

t2y added a commit to t2y/ldap that referenced this pull request Jul 20, 2023

fix: fix a deadlock issue when it occurs simultaneously: a goroutine …

735afe6

…is trying to lock, and another goroutine waits since full of channel buffers go-ldap#440

cpuschma pushed a commit that referenced this pull request Jul 22, 2023

fix: fix a deadlock issue when it occurs simultaneously: a goroutine …

d85ebd1

…is trying to lock, and another goroutine waits since full of channel buffers #440 (#446)

t2y mentioned this pull request Aug 15, 2023

Refactor DirSync search process #458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add search asynchronously with context #440

Add search asynchronously with context #440

t2y commented Jun 1, 2023 •

edited

cpuschma left a comment •

edited

cpuschma Jun 3, 2023

t2y Jun 4, 2023

t2y Jun 4, 2023

t2y Jun 4, 2023 •

edited

t2y Jun 4, 2023

t2y Jun 4, 2023

t2y Jun 4, 2023

cpuschma Jun 3, 2023

t2y Jun 4, 2023

t2y Jun 4, 2023

t2y commented Jun 4, 2023

t2y commented Jun 4, 2023

t2y commented Jun 5, 2023

t2y commented Jun 10, 2023

johnweldon commented Jun 12, 2023

t2y commented Jun 13, 2023

cpuschma commented Jun 15, 2023

t2y commented Jun 16, 2023

johnweldon commented Jun 19, 2023

t2y commented Jun 19, 2023

cpuschma commented Jun 21, 2023

t2y commented Jun 22, 2023

t2y commented Jun 22, 2023 •

edited

t2y commented Jun 22, 2023

cpuschma commented Jun 28, 2023

johnweldon left a comment

cpuschma commented Jun 30, 2023

Add search asynchronously with context #440

Add search asynchronously with context #440

Conversation

t2y commented Jun 1, 2023 • edited

References

cpuschma left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2y Jun 4, 2023 • edited

Choose a reason for hiding this comment

caller

search

Choose a reason for hiding this comment

client

search

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t2y commented Jun 4, 2023

t2y commented Jun 4, 2023

t2y commented Jun 5, 2023

t2y commented Jun 10, 2023

johnweldon commented Jun 12, 2023

t2y commented Jun 13, 2023

cpuschma commented Jun 15, 2023

t2y commented Jun 16, 2023

johnweldon commented Jun 19, 2023

t2y commented Jun 19, 2023

cpuschma commented Jun 21, 2023

t2y commented Jun 22, 2023

t2y commented Jun 22, 2023 • edited

t2y commented Jun 22, 2023

cpuschma commented Jun 28, 2023

johnweldon left a comment

Choose a reason for hiding this comment

cpuschma commented Jun 30, 2023

t2y commented Jun 1, 2023 •

edited

cpuschma left a comment •

edited

t2y Jun 4, 2023 •

edited

t2y commented Jun 22, 2023 •

edited