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

Studio: Reclaim unavailable port after site deletion #195

Merged
merged 7 commits into from
Jun 5, 2024

Conversation

SiobhyB
Copy link

@SiobhyB SiobhyB commented May 30, 2024

Fixes: 7366-gh-Automattic/dotcom-forge

Proposed Changes

  • With the changes in this PR, ports are now released when a site is deleted and the Studio app will re-use them if users create more sites.
  • The following main steps were necessary to achieve this:

Testing Instructions

  • Create a new site in the Studio app and make a note of the port via the local URL (e.g. 8881).
  • Verify the port is in use by running lsof -i :8881 in the command line (replace 8881 with your site's port).
  • In Studio, navigate to the Settings tab and delete the site.
  • Verify the port is now free by running lsof -i :8881 in the command line (again, replacing 8881 with your site's port).
  • Create another new site in the Studio app and verify that the app uses the port associated with the deleted site (i.e. 8881 in our examples).

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@SiobhyB SiobhyB self-assigned this May 30, 2024
.catch( ( err ) => {
console.error( `Failed to kill processes using port ${ port }: ${ err.message }` );
} )
.finally( () => {
Copy link
Author

Choose a reason for hiding this comment

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

These steps are added to the finally block as there are some instances where we can have the desired outcome even if killPort fails. For example:

  • User deletes site, but an error occurs when attempting to kill the port.
  • The user's laptop gets put to sleep, thus causing the port to be killed by the OS.
  • User returns to laptop and creates another site in the app.
  • The reclaimed port is used thanks to the fact it was removed from the #unavailablePorts list and #searchPort was reset to the default.

@@ -1,4 +1,5 @@
import http from 'http';
import killPort from 'cross-port-killer';
Copy link
Author

Choose a reason for hiding this comment

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

Before deciding to use this library, I tried a few things to try to release a CLOSED port for use, including:

  • Used http.createServer to attempt to .close the port explicitly. This led to the port remaining in a CLOSED state, but not being released, which is the same as what's already happening when a site is deleted (reference).
  • Experimented with explicitly setting socket.setReuseAddress(true), so as to be able to re-use the port more quickly and also detecting/destroying open sockets.

The only success I had was with manually killing the PIDs associated with a given port using a lsof command, which is only available for Unix systems. The cross-port-killer library works for all platforms and using it felt more efficient than the code that would be required.

I'm open to further thoughts or ideas here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining the reasoning behind using the library for that. It seems that the library is a wrapper and uses tools available in win32, linux, and darwin to kill the port, so it would go well with our support for Mac, Windows, and Linux in the future.

@SiobhyB SiobhyB marked this pull request as ready for review June 4, 2024 09:06
@SiobhyB SiobhyB requested review from a team June 4, 2024 09:06
@wojtekn
Copy link
Contributor

wojtekn commented Jun 4, 2024

@SiobhyB I tested it on macOS and couldn't make it work so far:

  1. Added the first site with 8906 port
  2. Added the second site with 8907 port
  3. Deleted the first site
  4. I noticed the message logged:
Killed processes using port 8906
  1. I waited about 15 seconds
  2. Added the third site, and it used port 8908

I would expect it used 8906, assuming all ports between 8881 and 8905 are taken.

Another thing I noticed is that Studio still doesn't reuse ports that were used in the past and are free now. Those ports are used by sites I have locally:

8881
8883
8885
8886
8887
8890
8894
8900
8902
8903
8904
8905
8907
8908

Studio keeps adding ports over the 8908, and doesn't reuse the free ports. It should work that way:

  1. Get a list of all ports claimed by existing sites
  2. Get a first free port starting from 8881
  3. If it's not claimed by existing site, use it, otherwise check the next port

Based on those, it should take 8882 in the above case. Any thoughts why it doesn't reuse those?

@SiobhyB
Copy link
Author

SiobhyB commented Jun 4, 2024

@wojtekn, thank you for spotting that! I found it was because #openPort was the value needing to be reset, rather than #searchPort, as the latter is being set using the value of the former. I've gone ahead to update in f1c9f97 and it's working in my testing now. Let me know if you spot anything else missing, though.

@SiobhyB SiobhyB requested a review from wojtekn June 4, 2024 13:26
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

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

LGTM 🎊 !

@wojtekn
Copy link
Contributor

wojtekn commented Jun 5, 2024

Thanks for fixing that @SiobhyB. The use case I shared works as expected now, and Studio now uses unused ports from the list I shared above.

@SiobhyB SiobhyB merged commit 5c06f42 into trunk Jun 5, 2024
11 checks passed
@SiobhyB SiobhyB deleted the fix/reclaim-unavailable-port-after-deletion branch June 5, 2024 09:40
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.

3 participants