go-ldap / ldap

Basic LDAP v3 functionality for the GO programming language.
Other
2.25k stars 355 forks source link

Add search asynchronously with context #440

Closed t2y closed 1 year ago

t2y commented 1 year ago

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.

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

t2y commented 1 year ago

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.

t2y commented 1 year ago

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

t2y commented 1 year ago

@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 commented 1 year ago

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

johnweldon commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

We have three options now.

  1. provides only SearchWithChannel() returns the channel
  2. provides only SearchAsync() returns the abstract Response interface
  3. provides both SearchWithChannel() and SearchAsync()

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

johnweldon commented 1 year ago

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

t2y commented 1 year ago

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

cpuschma commented 1 year ago

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

t2y commented 1 year ago

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

t2y commented 1 year ago

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

t2y commented 1 year ago

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

cpuschma commented 1 year ago

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

cpuschma commented 1 year ago

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