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

Refactor port detection code #113

Open
puzrin opened this issue Jan 16, 2016 · 8 comments
Open

Refactor port detection code #113

puzrin opened this issue Jan 16, 2016 · 8 comments

Comments

@puzrin
Copy link
Contributor

puzrin commented Jan 16, 2016

Current port detection code tries to be compact and simple. As a result - logic partially goes to shell, and becomes messy.

  • It's not evident when shell script failed (some commands not support use params in certain env).
  • Fixing complex command can break it for another env (shells can be different).

Ideas:

  1. If anyone have time, it would be nice to split this logic into separate part.
  2. If anybody wish, it would be nice to configure more test envs to make sure that port detection works.

update

  • 2.0 should support port reporting. So, just drop that code when phantomjs 2.0 is available on npm.
  • Problems with bad -p key are related to docker busybox images. Can be solved by install full version of netstat/ss
@baudehlo
Copy link
Owner

A better option would be to get phantomjs to report the actual port used
when using port 0. That way we could just ask for it directly. I had hoped
they would fix this in phantomjs 2.0, but I guess not?

On Sat, Jan 16, 2016 at 8:28 AM, Vitaly Puzrin [email protected]
wrote:

Current port detection code tries to be compact and simple. As a result -
logic partially goes to shell, and becomes messy.

  • It's not evident when shell script failed (some commands not support
    use params in certain env).
  • Fixing complex command can break it for another env (shells can be
    different).

Ideas:

  1. If anyone have time, it would be nice to split this logic into
    separate part.
  2. If anybody wish, it would be nice to configure more test envs to
    make sure that port detection works.


Reply to this email directly or view it on GitHub
#113.

@puzrin
Copy link
Contributor Author

puzrin commented Jan 16, 2016

Ah, you are right. Alternative is to completely drop this code, when 2.0 available. Forgot about it :)

Current situation is, that phantom 2.0 can not be installed on linux via npm. This problem is known, old, and nobody can say when it will be solved. So, i did not tried to investigate what's up with port in 2.0.

@puzrin
Copy link
Contributor Author

puzrin commented Jan 25, 2016

Just for records. Reports about bad -p key are related to docker busybox images. http://serverfault.com/questions/219984/busybox-netstat-no-p. Can be solved by install of full netstat/ss

@puzrin
Copy link
Contributor Author

puzrin commented Jan 25, 2016

https://github.com/Medium/phantomjs/releases/tag/v2.1.1

Holy shit! PhantomJS 2.1 finally available on npm for linux!

@puzrin
Copy link
Contributor Author

puzrin commented Jan 26, 2016

Update

2.1.2 works somehow, but:

  1. Need to polish tests
    • one still broken here (onError)
    • one still broken in navit (after patches)
  2. New version sends cache headers, and that can fuckup everything, because you can get 304 response (without body/headers) instead of expected 200 response.

Will continue tomorrow.

@puzrin
Copy link
Contributor Author

puzrin commented Jan 26, 2016

Nothing changed about port in 2.1. Created issue ariya/phantomjs#13947

@puzrin
Copy link
Contributor Author

puzrin commented Feb 16, 2016

Good news - issue assigned for 2.2 release

@RobLoach
Copy link

RobLoach commented Apr 6, 2016

Should this be targeting phantomjs-prebuilt?

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

No branches or pull requests

3 participants