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

Added support for /etc/hosts #36

Closed
wants to merge 3 commits into from
Closed

Conversation

jaapio
Copy link

@jaapio jaapio commented Mar 4, 2016

This adds support for /etc/hosts through a HostsFileExecutor executor as requested in #10

The Factory is changed so that the Resolver looks in /etc/hosts first. The executor stack looks like this: CachedExecutor -> HostsFileExecutor -> RetryExecutor -> Executor.

Thanks to @arnaud-lb for creating the original PR reactphp/reactphp#225

This adds support for /etc/hosts through a HostsFileExecutor executor.

The Factory is changed so that the Resolver looks in /etc/hosts first. The executor stack looks like this: CachedExecutor -> HostsFileExecutor -> RetryExecutor -> Executor.
@cboden
Copy link
Member

cboden commented Mar 4, 2016

Awesome work! However reading a file is blocking. I would suggest implementing one of the following changes:

  1. loadHosts() in the __construct and save the promise to a property. If the developer is inclined to refresh later, albeit blocking, then can do so, replacing the promise property. query() would look like $this->hosts->then(...

  2. Use reactphp/filesystem to asynchronously read the file

@jaapio
Copy link
Author

jaapio commented Mar 4, 2016

will have a look at that. however that will introduce a new dependency? Not sure if that will be a problem. and resolve.conf has the same issue?

@cboden
Copy link
Member

cboden commented Mar 4, 2016

Yeah...just checked out filesystem...no problem with the extra lib dependency but it requires the EIO extension...so I would suggest going with 1. resolve.conf would have the same issue, yeah

@jaapio
Copy link
Author

jaapio commented Mar 4, 2016

@cboden this is what I understood from your comment.
react is quite new for me so if I need to do any thing else I might need some more help.

@WyriHaximus
Copy link
Member

but it requires the EIO extension

Actually, my recent work removed the dependency on it. Still fine tuning it, but it works without ext-eio 😄

$this->loop = $loop;
$this->executor = $executor;
$this->path = $path;
$this->hosts = $this->loadHosts();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just call $this->loadHosts()

@clue
Copy link
Member

clue commented Mar 12, 2016

👍 I'd love to see this feature in – however I'm not sure about the limitations imposed by the current implementation:

  • The fopen() call is still blocking
  • Only the StreamSelectLoop supports watching for file system events (the other event loop implementations will print a bunch of warnings)
  • The child-process component introduces an additional dependency (which is okay), however the EIO extension it not generally available and the child-process adapter will likely have issues on Windows.

My personal vote would be: Do we really care if loading the file is blocking? The results will be kept in memory anyway, so perhaps we can make this an explicit choice so that the blocking calls can be performed before the loop runs?

@cboden
Copy link
Member

cboden commented Apr 27, 2016

I'm +1 for having an initial load that is blocking before the loop gets going in order to get this expected behaviour in. We can refactor to make it pure async at a later point in time.

@WyriHaximus
Copy link
Member

@cboden that works for me 👍

@kuai6
Copy link

kuai6 commented Jun 4, 2017

When it will be done?

@cboden cboden requested review from cboden and removed request for cboden September 9, 2017 21:19
@clue
Copy link
Member

clue commented Sep 12, 2017

@jaapio Thank you for filing this PR and sparking the discussion again! 👍

Given that this PR hasn't received an update in a while, we've implemented this feature as part of #75 which landed in the last release :shipit:

I believe this has been answered, so I'm closing this for now. Again, thank you for your contribution and keep it coming! 👍

@clue clue closed this Sep 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants