Bug 66842 – [WebSocket] Terminate existing run-webkit-websocketserver at the next run

• Component: Tools / Tests
• Status: UNCONFIRMED

[Takashi Toyoshima]

Sometime, test infrastructure fails to launch new WebSocket server because of unexpected existing server.
It was problem when we update pywebsocket.

This change enables to terminate existing WebSocket server before launch new one.

[Takashi Toyoshima]

Created attachment 104971 [details]
Patch

[Yuta Kitamura]

I think new-run-webkit-websocketserver already checks pidfile and tries to kill the existing one.

> test infrastructure fails to launch new WebSocket server because of unexpected existing server.

Dirk improved new-run-webkit-websocketserver a few months ago, and websocketserver starts up far more reliably these days.

> It was problem when we update pywebsocket.

I don't think this is the case now.

[Yuta Kitamura]

Comment on attachment 104971 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104971&action=review

> Tools/ChangeLog:8
> +        * Scripts/run-webkit-websocketserver:

I think these logic should be done inside our python packages "webkitpy.layout_tests.servers.*", not in run-webkit-websocketserver.

[Takashi Toyoshima]

run-webkit-websocketserver kicks new-run-webkit-websocketserver internally.
But, in my local trial, the second run-webkit-websocketserver could not kill the first one.

For example,
1. run-webkit-websocketserver in terminal A
2. run-webkit-websocketserver in terminal B
3. hit enter to stop the server in terminal A
4. nobody listen at 8880

run-webkit-httpd also has a similar killing code.

[Yuta Kitamura]

(In reply to comment #4)
> run-webkit-websocketserver kicks new-run-webkit-websocketserver internally.
> But, in my local trial, the second run-webkit-websocketserver could not kill the first one.
> 
> For example,
> 1. run-webkit-websocketserver in terminal A
> 2. run-webkit-websocketserver in terminal B
> 3. hit enter to stop the server in terminal A
> 4. nobody listen at 8880

It actually does.

1. A new pywebsocket process (PID=X) is up.
2. run-webkit-websocketserver kills the existing server (PIX=X), and starts a new one (PID=Y).
3. The new server (PID=Y) is get killed, because pidfile is overwritten in step 2.

So in this case, the problem is in step 3. I don't think your patch fixes this.

> 
> run-webkit-httpd also has a similar killing code.

Libraries under webkitpy already take care of it. You shouldn't touch this perl script.

[Takashi Toyoshima]

All right, I see the situation.

So problem is that the parent process, run-webkit-websocketserver doesn't die when pywebsocket is killed, isn't it?
First, I tried to rewrite <STDIN> to waitpid($oldPid, 0) or hook SIGCHLD. But both of them didn't work.

Anyway, at least we need a change to shutdown only self launched server after hitting enter.

[Takashi Toyoshima]

Created attachment 105135 [details]
Patch

[Takashi Toyoshima]

As Yuta-san mentioned, this is NOT a real problem.
I changed its importance to P5 minor.

But, I already make alternative change to terminate run-webkit-websocketserver itself.
It could be helpful to use it by manual.

[Ojan Vafai]

Comment on attachment 105135 [details]
Patch

I didn't follow all the discussion here, but it sounds like this is not a problem in practice. I'm r-'ing for now to remove it from the review queue.  Takashi-san, please mark for review again if you think this should still be committed. 

Yuta-san, if you do an informal review for this, I'm happy to r+ it.

[Yuta Kitamura]

I'm not convinced of the necessity of this change yet.

pidfile is already created and managed elsewhere, so adding another pidfile doesn't sound right to me. My gut feeling is that this kind of logic should go into webkitpy, not in this perl file.