Studio: Check if port is open before starting servers - #1236
Conversation
| return new Promise( ( resolve ) => { | ||
| const server = http.createServer(); | ||
| // First try to connect to the port | ||
| const socket = new net.Socket(); |
There was a problem hiding this comment.
The previous isPortFree() implementation used http.createServer().listen() to check port availability. However, this was leading to false positives. The port was showing free when it was actually in use. This might be happening because listen() may not immediately fail if the port is in a TIME_WAIT state or due to OS-level race conditions.
The updated logic first attempts to connect to the port using a socket to improve reliability. A successful connection confirms the port is in use. If the connection fails, we try to bind to the port to to make sure it's truly available.
There was a problem hiding this comment.
I have tested it, and it works as advertised. I love this change as it improves the UX when there are some errors, and those errors of port in use are very common so this is 💯 . I have left a non-blocking comment about the style of a method but overall this LGTM!
| Port in use for existing site | Having the next port in use for new site |
|---|---|
 |
 |
| getIpcApi().showErrorMessageBox( { | ||
| title: __( 'Failed to start the site server' ), | ||
| message: __( | ||
| `The site server failed to start because the port is already in use. Please close any local development apps that may be using port ${ port } and try again.` |
There was a problem hiding this comment.
One idea that comes to mind as a potential improvement even further could be that instead of the Ok button as shown now, we could add a button that says something like Try another port and it could automatically attempt to start the server on a different port.
There was a problem hiding this comment.
This pattern is also used by some build tools, without prompting the user. Eg. when using Vite, if the default port is occupied, it just increments the dev port until it finds a free port.
There was a problem hiding this comment.
Thanks for the suggestion, @katinthehatsite. This was one of the suggestions in the linked issue. However, it would require a larger effort than the current PR. I believe this PR improves the experience significantly.
|
The testing steps worked for me and the implementation works as expected 👍 I added one suggestion that we can consider to potentially improve the user experience further. |
* Update a way to check if port is open * Check if port is open before starting servers * Do not show open logs button * Accept port as args in #isPortFree
* Update a way to check if port is open * Check if port is open before starting servers * Do not show open logs button * Accept port as args in #isPortFree
|
We also have |
|
Thanks @fredrikekelund for bringing it to my attention. I don't see any reason not to refactor |
|
For posterity, I have created a PR #1281 to address the above suggestion. |
6 participants
Related issues
Proposed Changes
Testing Instructions
npm startphp -S localhost:{PORT_NUMBER}. e.g.php -S localhost:8881Startbutton.php -S localhost:8881, press cmd + c to terminate the server. This would free up port 8881.Bonus Test
appdata-v1.json(cat "~/Library/Application Support/Studio/appdata-v1.json")siteskey, note down the highest occupied port number. For example, in my case it's8883.8884php -S localhost:8884Pre-merge Checklist