search

inejge / ldap3

A pure-Rust LDAP library using the Tokio stack
Apache License 2.0
220 stars 38 forks source link

any way to set the timeout? #4

Closed bbigras closed 7 years ago

bbigras commented 7 years ago

Is there any way to set a custom timeout?

inejge commented 7 years ago

Not yet. I'm planning to introduce it in 0.5.x or 0.6.x for the synchronous API. I'm not sure about the async API, and I'll have to see how timeouts, delayed responses, and tokio-proto multiplex machinery interact.

ndenev commented 7 years ago

This would be great addition. Right now this is the only thing that prevents me from using this great crate.

inejge commented 7 years ago

Initial timeout support is in 4b6426811b09b6fa695c6c0de735fde1f52c8ecb. If you're feeling adventurous, try it out. There's a small example in examples/search_timeout.rs which shows how to use timeouts in the code.

Cc @bbigras @ndenev

ndenev commented 7 years ago

@inejge I've tried only the connect timeout, but it seems to work perfectly. Thanks!

bbigras commented 7 years ago

I tried in sync mode. connect and search timeouts seems to work but for some reason I got the connect timeout error in the result and the search timeout as rc code 85 (the result was Ok()).

inejge commented 7 years ago

This is intentional, since returning a search timeout as an Err through the futures chain messed up the connection for subsequent operations, somewhere in the depths of tokio-proto. The next version of the driver will have a helper method to convert the returned LdapResult into an Err of itself if rc is nonzero, so error handling can be a bit more idiomatic.

Update: It's possible that the connection problem in the Err case was caused by incomplete clean-up on my part, but I'll probably keep the dual timeout reporting for now, as it makes managing tokio-proto resources easier.

bbigras commented 7 years ago

Great. Thanks for implementing this I was also waiting for this feature to switch from another crate. I guess we can close this issue now.

inejge commented 7 years ago

I'd prefer leaving it open for a little while, until the timeout implementation stabilizes a bit. I've just pushed a commit (401ca031ecec0191eedb9e72921ddee2eb78c154) which goes back to reporting all timeouts via Err (it was an oversight on my part). Thanks for the great feedback!

inejge commented 7 years ago

Timeout handling is part of the 0.5.0 release, so closing this now.