Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-blocking reads with configurable timeouts #167

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

mtodd
Copy link
Member

@mtodd mtodd commented Dec 1, 2014

Rewire IO#read calls originating from the Net::BER::BERParser module to use non-blocking reads, depending on IO.select to block until the socket is readable or times out, raising a timeout error (instead of blocking forever; though that is still the default).

  • non-blocking reads
  • configurable timeout
  • extensive unit & integration tests

The internal and external APIs are likely to change (feedback welcome). Would welcome general thoughts or concerns for these kinds of changes. From the recent research I've been doing, this seems to be the most reliable, forward-compatible way to implement non-blocking reads (using IO.select, as opposed to setsockopt with SO_RCVTIMEO).

Pairing this with similar changes for write would be the logical next step, assuming this makes sense. It's worth considering making non-blocking/blocking calls configurable; this might be necessary since it's not always needed and is likely to behave in unexpected ways, so we'd not want that to be a surprise for anybody, or worse: have no recourse if it breaks their integration surprisingly.

I'm still thinking through what the end-result this would afford us, especially in terms of how the Net::LDAP API can change (search_async, for instance). This PR is pretty superficial (no major changes), but is a good step towards introducing more asynchronous, non-blocking API designs.

Thoughts?

see #164
cc @jch @schaary

@mtodd
Copy link
Member Author

mtodd commented Dec 1, 2014

Build failing due to #166, but is green otherwise, which tells me we probably need better tests for these code paths.

@jch
Copy link
Member

jch commented Dec 15, 2014

I like the idea of non-blocking I/O, and I agree this PR is a good small step for us to move in that direction. I'm less interested in exposing public API like search_async, instead preferring non-blocking I/O concerns to be internal and private. I don't think net-ldap users should know how I/O is implemented.

How does this affect other rubies and platforms? Officially, I think we only have the bandwidth to maintain MRI rubies and Unix-like systems because that's what we're using, but where reasonable, I'd like to keep it easy for other contributors. I think we should keep exploring this route, but if it breaks jruby or windows, maybe we can wrap this functionality into a module.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants