ldap notifications: add SearchAsync #341
Conversation
|
oops I did not see #319, sorry 😬 |
Adphi
force-pushed
the
search-async
branch
2 times, most recently
from
92e9bfd to
2c6cba8
Compare
|
Sorry for the delay, I'll look into this this week! |
|
Hi any news on this? |
search.go
Outdated
| @@ -402,6 +439,77 @@ func (l *Conn) Search(searchRequest *SearchRequest) (*SearchResult, error) { | |||
| } | |||
| } | |||
|
|
|||
| // SearchAsync performs the given search request asynchronously, it returns a SearchAsyncResponse channel which will be | |||
| // closed when the request finished and an error, not nil if the request to the server failed | |||
| func (l *Conn) SearchAsync(ctx context.Context, searchRequest *SearchRequest) (<-chan *SearchAsyncResponse, error) { | |||
There was a problem hiding this comment.
There's currently a discussion in another PR regarding context.Context (#406). This would be the only function to accept a context. I suggest to cut out the context handling and wait for the mentioned PR to be merged so we have a streamlined design on how to implement context.Context.
There was a problem hiding this comment.
Sorry about that. I'll remove it, but we need a way to stop the receive loop.
Maybe take an optional done channel as parameter to stop when it is closed, e.g.:
// SearchAsync performs the given search request asynchronously, it takes an optional done channel to stop the request. It returns a SearchAsyncResponse channel which will be
// closed when the request finished and an error, not nil if the request to the server failed
func (l *Conn) SearchAsync(searchRequest *SearchRequest, done chan struct{}) (<-chan *SearchAsyncResponse, error)
search.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| responses := make(chan *SearchAsyncResponse, 1) |
There was a problem hiding this comment.
I dislike the idea to arbitrary limit or hardcode the channel here to a size of one. This would result in a blocking reader in case the function which called SearchAsync is processing the results or blocking due to other reasons. If we're working with a paging cookie, the LDAP DS could free the cookie to prevent Denial of Service and fetching further results would fail
Any thoughts on this, @go-ldap/committers?
There was a problem hiding this comment.
Your concerns seem reasonable to me @cpuschma; would a blocking call on the client side cause resources on the server side to be tied up indefinitely? What would be the difference to the server (in terms of resource usage, and potential ddos) if it was an unbuffered channel and the reader on the client just stopped reading?
There was a problem hiding this comment.
Directory servers have a short lease duration for paging cookies, @johnweldon. The overhead for the directory server is therefore very low, unless you intentionally start thousand paging requests without filters. It is more likely that the server will release the cookie. Queries with the PagingControl and the cookie will most likely fail.
There was a problem hiding this comment.
@Adphi - what is the benefit of a buffered channel here?
There was a problem hiding this comment.
I can't remember why, and I don't see any benefit of using a buffered channel here. I'll remove it.
Adphi
force-pushed
the
search-async
branch
2 times, most recently
from
337a51e to
bbd6039
Compare
Signed-off-by: Adphi <[email protected]>
* feat: add search with channels inspired by #319 * refactor: fix to check proper test results #319 * refactor: fix to use unpackAttributes() for Attributes #319 * refactor: returns receive-only channel to prevent closing it from the caller #319 * refactor: pass channelSize to be able to controll buffered channel by the caller #319 * fix: recover an asynchronouse closing timing issue #319 * fix: consume all entries from the channel to prevent blocking by the connection #319 * feat: add initial search async function with channel #341 * feat: provide search async function and drop search with channels #319 #341 * refactor: lock when to call GetLastError since it might be in communication
Add a SearchAsync function needed to implement ldap notifications (#302).