search

celluloid / celluloid-io

UNMAINTAINED: See celluloid/celluloid#779 - Evented sockets for Celluloid actors
https://celluloid.io
MIT License
879 stars 93 forks source link

Let's use some undocumented Java APIs to make the code work as it should... #65

Closed perlun closed 11 years ago

perlun commented 11 years ago

... on JRuby/Windows.

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling a5c3acde9a16485bd1b483a3ab534a36bc3e3e36 on perlun:dns_resolver_jruby_windows into df768cd3cdb2747512cab56f71a8d9b68e9f9161 on celluloid:master.

halorgium commented 11 years ago

Using a strategy pattern here might make more sense. Less conditionals.

perlun commented 11 years ago

Care to elaborate? Or even better, PR my PR. :)

tarcieri commented 11 years ago

Can't you obtain the current DNS servers via ipconfig?

halorgium commented 11 years ago

@perlun A class with the specific method implemented for each different type.

perlun commented 11 years ago

Yeah, but I don't know if its so much cleaner. :) Works on MRI also though, which is an obvious plus.

Sent from my iPhone

On 28 maj 2013, at 23:41, "Tony Arcieri" notifications@github.com<mailto:notifications@github.com> wrote:

Can't you obtain the current DNS servers via ipconfig?

— Reply to this email directly or view it on GitHubhttps://github.com/celluloid/celluloid-io/pull/65#issuecomment-18578297.

tarcieri commented 11 years ago

It would be one codepath regardless, which to me seems like a big win.

The trivial version could just hit ipconfig for each resolution request. Bonus points for a short-lived cache of domain names.

halorgium commented 11 years ago

@tarcieri careful about caching. dislike caching in app code. nothing worse than buggy DNS cache code.

tarcieri commented 11 years ago

I think the cache is fine if it's extremely short lived (5 seconds... or even 1 second)

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling b2fa52dfda28118d833f2feb98359503653c2530 on perlun:dns_resolver_jruby_windows into df768cd3cdb2747512cab56f71a8d9b68e9f9161 on celluloid:master.

perlun commented 11 years ago

I fixed a basic version of the 'ipconfig /all' approach, which should work on both MRI and JRuby. Ugly in my eyes, and even worse - I'm not sure it will work correctly on a non-English version of Windows...

Nonetheless, here it is. I think it would be better to use the Java API when on JRuby and use a Win32 API in MRI...

@halorgium: I'll gladly switch over to a strategy-based approach. Any suggestions to how it can be done? A separate class used by DNSResolver? (its instantiation still has to be conditionalized though, but it might still be slightly cleaner.)

tarcieri commented 11 years ago

@perlun I can factor this into multiple providers if you're having trouble. Forget the ipconfig cache, let's just use ipconfig as a fallback implementation, and implement some other more elegant providers we can use if available

perlun commented 11 years ago

@tarcieri: OK. I'll work a bit more on this in the direction as you suggested - thanks for the feedback! Will let you know when it is ready for peer review/refactoring.

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling 80f2106e12d4fad57ecb6d1f46881c65551cb335 on perlun:dns_resolver_jruby_windows into 6004dff0aedfc2b25c55329c6db52fb5ae5fbb85 on celluloid:master.

tarcieri commented 11 years ago

@perlun check out @Dparker1990's work, he's trying to move to Resolv::Hosts: https://github.com/celluloid/celluloid-io/pull/73

perlun commented 11 years ago

@tarcieri Good point. I looked at it and it doesn't solve the same problem, but it's definitely good and I believe we should try to merge both of these PRs for 0.15.

The specs work with my branch now on JRuby Windows and JRuby Mac OSX. Didn't get rspec working on MRI/OSX for some reason but I guess Travis will let us know in a few minutes... :)

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling 23de5a16e5cedf6434c71eab8652afed2d239468 on perlun:dns_resolver_jruby_windows into 6004dff0aedfc2b25c55329c6db52fb5ae5fbb85 on celluloid:master.

tarcieri commented 11 years ago

@perlun cool, I'd like to land @Dparker1990's patch first then revisit yours

perlun commented 11 years ago

@tarcieri Fine by me. Please if you remember, poke me when it has been merged so I can pull in his changes into my tree. Btw, there was a regression with MRI that Travis told me about so I fixed that also.

coveralls commented 11 years ago

Coverage Status

Coverage remained the same when pulling bbcc213cafdb3e6c2a48f7f3cad628f431b1ea9b on perlun:dns_resolver_jruby_windows into 6004dff0aedfc2b25c55329c6db52fb5ae5fbb85 on celluloid:master.

tarcieri commented 11 years ago

This issue is out-of-date and the DNS resolver code has changed considerably

perlun commented 11 years ago

OK, I'll see if it get the time/energy to try to rebase the fixes on top of the latest master. Was awaiting the @DParker1990 PR to be merged but I presume this has happened now?

tarcieri commented 11 years ago

Yes. Try to rebase if you can, but a lot has changed

perlun commented 11 years ago

Yeah, it will probably not be a true rebase in the git sense but more a "logical rebase" off of the new head.