Allow bridge host and port to be specified via options

[w33ble]

I recently ran in to an issue in Docker (like several other people have, based on what I've seen) where netstat and ss don't actually return the port used, and node-phantom-simple falls apart. We found that privileged containers worked fine, but we needed to run unprivileged containers.

Of course there have been several issues here related to netstat/ss for port detection based on pid.

Long story short, the problem doesn't actually seem to be with Phantom, but instead with the bridge.

We discovered that this library was binding the bridge server to port 0, and that there was already code to handle the port not being 0. So I tried passing the port into the bridge using the argument passthrough in the phantom cli and everything worked as expected in our unprivileged containers.

This doesn't change the default operation of node-phantom-simple, it simply allows users to pass in the bridge.host and bridge.post as options in driver.create().

I don't know if this will solve all of the port problems for others as well, and I'll admit that I only tested this with Phantom 1.9.8, but I haven't run in to any issues specifying my own port yet. I included the host, but I'm not sure there's a need to, the port was all that seemed important here. I would also be open to flattening the options to something like bridgeHost and bridgePost, or whatever you'd prefer.

This seems really useful in upstream here, and I'm happy to update this PR as you see fit.

[w33ble]

Looks like tests are failing due to do the Slimer download responding with 403. I ran the tests locally and they all passed. I'm not sure what's going on with Travis here.

[baudehlo]

But presumably this breaks under cluster (hence the reason for the current implementation)

On Sep 1, 2016, at 8:42 PM, Joe Fleming notifications@github.com wrote:

I recently ran in to an issue in Docker (like several other people have, based on what I've seen) where netstat and ss don't actually return the port used, and node-phantom-simple falls apart. We found that privileged containers worked fine, but we needed to run unprivileged containers.

Of course there have been several issues related here to netstat/ss for port detection based on pid.

Long story short, the problem doesn't actually seem to be with Phantom, but instead with the bridge.

We discovered that this library was binding the bridge server to port 0, and that there was already code to handle the port not being 0. So I tried passing the port into the bridge using the argument passthrough in the phantom cli and everything worked as expected in our unprivileged containers.

This doesn't change the default operation of node-phantom-simple, it simply allows users to pass in the bridge.host and bridge.post as options in driver.create().

I don't know if this will solve all of the port problems for others as well, and I'll admit that I only tested this with Phantom 1.9.8, but I haven't run in to any issues specifying my own port yet. I included the host, but I'm not sure there's a need to, the port was all that seemed important here. I would also be open to flattening the options to something like bridgeHost and bridgePost, or whatever you'd prefer.

This seems really useful in upstream here, and I'm happy to update this PR as you see fit.

You can view, comment on, or merge this pull request online at:

https://github.com/baudehlo/node-phantom-simple/pull/141

Commit Summary

pass bridge options to phantom exec use passed host and port in bridge File Changes

M bridge.js (4) M node-phantom-simple.js (4) Patch Links:

https://github.com/baudehlo/node-phantom-simple/pull/141.patch https://github.com/baudehlo/node-phantom-simple/pull/141.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[puzrin]

AFAIK, general strategy of this driver is to allow run multiple phantoms with high frequency. The only way to avoid port collisions without races was to use built-in resolver. So any attempts to allow pass port/host were rejected in the past as wrong direction.

Is there any other way to detect phantom port in your docker image, may be another network utilities? I'm not experienced with docker.

PS. Don't pay attention to slimerjs problem.

[puzrin]

@w33ble see #113.

• i don't like having kluge with port in API.
• from the other hand, bug in phantomjs in not fixed for a long time and i understand people who ask to add this kludge.

@baudehlo may be we could add this option as undocumented/unstable, for "experts"?

[baudehlo]

Yeah the option is a possibility but there MUST be a way to detect the port in docker images. Even if it's a direct probe of /proc or something.

On Sep 1, 2016, at 9:18 PM, Vitaly Puzrin notifications@github.com wrote:

@w33ble see #113.

i don't like having kluge with port in API. from the other hand, bug in phantomjs in not fixed for a long time and i understand people who ask to add this kludge. @baudehlo may be we could add this option as undocumented/unstable, for "experts"?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[w33ble]

The only way to avoid port collisions without races was to use built-in resolver.

That's true. The window between random port detection and starting the phantom instance is pretty small, but yes, there is a chance for overlap there.

However, the user has to opt in to the problem (like I have), as the default method of letting everything start up and then detecting the port hasn't changed. This PR simply introduces the option.

Is there any other way to detect phantom port in your docker image, may be another network utilities? I'm not experienced with docker.

I probably have less experience than you do, most of my Docker info was coming from people who were more experienced, and they weren't aware of a workaround for querying the process. I don't know if /proc is accessible for probing, and I don't know any alternatives.

[puzrin]

@w33ble why don't you install package with one of those utility to your docker image?

[puzrin]

Any feedback? I tend to reject.