search

go-ldap / ldap

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

Support context.Context #326

Open asuffield opened 3 years ago

asuffield commented 3 years ago

This module currently lacks any support for context.Context, so it can't inherit conventional timeout and cancellation semantics. That seems worth adding.

Adphi commented 1 year ago

I have the beginning of a PR for context cancelation. One blocking point is the behavior on cancel. Should it become an abandon request? This one was not implemented here last time I checked.

Joh4nnesHartl commented 1 year ago

Definitely would be a nice feature, I am using context.Context almost everywhere and have some ugly wrapper around the ldap client. Support for it would be great.

Joh4nnesHartl commented 1 year ago

SUGGESTION:

How about enhancing the *ldap.Conn structure with new methods that end with WithContext to introduce conventional timeout & cancellation semantics to go-ldap.

Example:

// Add performs the given AddRequest
func (l *Conn) Add(addRequest *AddRequest) error

// AddWithContext performs the given AddRequest with a context, the context controls the entire lifetime of a request and its response
func (l *Conn) AddWithContext(ctx context.Context, addRequest *AddRequest) error

// Search performs the given search request
func (l *Conn) Search(searchRequest *SearchRequest) (*SearchResult, error)

// SearchWithContext performs the given search request with a context, the context controls the entire lifetime of a request and its response
func (l *Conn) SearchWithContext(ctx context.Context, searchRequest *SearchRequest) (*SearchResult, error)

...

This would ensure API compatability to the old methods and would not cause breaking changes for an upcoming patch like this. There would be another solution I thought of but it would introduce BREAKING CHANGES, which are most likely not wanted.

cc @Adphi @johnweldon @cpuschma

johnweldon commented 1 year ago

How about enhancing the *ldap.Conn structure with new methods that end with WithContext to introduce conventional timeout & cancellation semantics to go-ldap.

I like this suggestion in principle. I'm unclear about actual implementation of the timeout and cancellation logic, but there should be ample examples and suggestions in other projects.

asuffield commented 1 year ago

I'll knock out an attempt at an initial implementation

Joh4nnesHartl commented 1 year ago

I'll knock out an attempt at an initial implementation

I can join if you want :+1:

asuffield commented 1 year ago

Here's a first cut: https://github.com/go-ldap/ldap/pull/406

Not mirrored into v3 (yet), just bashing out what the basic API would look like. Probably still buggy.

cpuschma commented 1 year ago

The same concept has been implemented by other libraries, each method has an equivalent with the option to provide a context.

I would follow the same scheme here