Avoid port conflicts #73

dcdenu4 · 2020-12-02T13:06:20Z

This PR attempts to solve the problem of launching the Flask App on a port that is already in use. It does this by:

using a Python socket to check if a port is available.
If a port is not available, it tries the next port number for n amount of ports.
The client side which spawned the Flask App gets the used port number via listening to the spawned processes stdout.

This does NOT solve the problem of launching multiple Workbench UIs, which Dave and I discussed should be a separate issue.

dcdenu4 · 2020-12-02T19:47:03Z

src/main_helpers.js

@@ -33,6 +33,7 @@ export function findInvestBinaries(isDevMode) {
      // If no dotenv vars are set, default to where this project's
      // build process places the binaries.
      serverExe = `${process.env.SERVER || 'build/invest/server'}${ext}`;
+      //serverExe = `${process.env.SERVER || 'build/invest/server'}`;


I left this commented code here, because when I was running in dev mode using .env and server.py the auto adding of ext was causing issues. Should the resolving line be:

serverExe = `${process.env.SERVER}` || `build/invest/server${ext}`;

Yeah, I think the idea was to autocomplete the ext even for the .env path. But that seems like a bad idea now. If someone is putting a path into .env, the natural thing to do is include the extension there explicitly. We should make it consistent and also change investExe on the next line.

Think I have it updated here: b592fc7

davemfish

Hey @dcdenu4 - thanks for working on this, it turned out to be a trickier problem than I realized back when the issue was created.

I have a few comments and suggestions here. They're all fairly minor - I think the general solution here is great. A few minor comments that I think might have been caught/warned about by eslint. Could you try that if you haven't already?

When the server.py changes get ported over to invest, we'll probably want to add to test_cli.py to cover the new command-line args you added here.

Oh, and could you look into the failing npm run test-flask-app? It might just need a minor change since it also calls createPythonFlaskProcess

Thanks!

davemfish · 2020-12-03T14:07:00Z

src/main.js

@@ -36,7 +34,8 @@ const createWindow = async () => {
    event.reply('variable-reply', mainProcessVars);
  });

-  createPythonFlaskProcess(binaries.server, isDevMode);
+  // Wait for a response from the server confirming the host information
+  await createPythonFlaskProcess(binaries.server, isDevMode);
  // Wait for a response from the server before loading the app
  await getFlaskIsReady();


We talked about these awaits and this seems right. Could we just differentiate these comments a bit? The first one being like "Wait for a response from python confirming the host info" and then the second one being something like "Wait for the server to finish its startup"

Updated here: addc71e

davemfish · 2020-12-03T14:11:04Z

src/main_helpers.js

@@ -33,6 +33,7 @@ export function findInvestBinaries(isDevMode) {
      // If no dotenv vars are set, default to where this project's
      // build process places the binaries.
      serverExe = `${process.env.SERVER || 'build/invest/server'}${ext}`;
+      //serverExe = `${process.env.SERVER || 'build/invest/server'}`;


Yeah, I think the idea was to autocomplete the ext even for the .env path. But that seems like a bad idea now. If someone is putting a path into .env, the natural thing to do is include the extension there explicitly. We should make it consistent and also change investExe on the next line.

davemfish · 2020-12-03T14:14:00Z

src/main_helpers.js

-        env: { PATH: path.dirname(serverExe) },
+export async function createPythonFlaskProcess(serverExe, isDevMode) {
+  var isPort = false;
+    if (serverExe) {


uneccessary indent here?

davemfish · 2020-12-03T14:15:54Z

src/main_helpers.js

-      pythonServerProcess = spawn(path.basename(serverExe), {
-        env: { PATH: path.dirname(serverExe) },
+export async function createPythonFlaskProcess(serverExe, isDevMode) {
+  var isPort = false;


Do you like var here? We tend to use let in this context. let limits the name to block scope.

davemfish · 2020-12-03T14:21:29Z

src/main_helpers.js

+      if (isDevMode && process.env.PYTHON && serverExe.endsWith('.py')) {
+        // A special devMode case for launching from the source code
+        // to facilitate debugging & development of src/server.py
+        var port = `${process.env.PORT || '5000'}`;


I think this var could probably be const, right? If it never gets re-assigned

davemfish · 2020-12-03T14:26:45Z

src/main_helpers.js

+        isPort = strData.includes('PORT');
+        if (isPort) {
+            let idx = strData.indexOf('PORT');
+            let flaskPort = strData.slice(idx+5, idx+9);


I think all the lets in this listener could technically be const

davemfish · 2020-12-03T14:32:56Z

src/main_helpers.js

+      // Try every X ms, usually takes a couple seconds to startup.
+      await new Promise((resolve) => setTimeout(resolve, 500));
+      logger.debug(`Waiting for Port confirmation: retry # ${i}`);
+    }


Do you think the retry stuff here should go inside the big if (serverExe) block? If somehow there is no serverExe, we wouldn't need to bother with these retries.

davemfish · 2020-12-03T14:43:00Z

src/main_helpers.js

-      // also putting it's location on the PATH:
-      pythonServerProcess = spawn(path.basename(serverExe), {
-        env: { PATH: path.dirname(serverExe) },
+export async function createPythonFlaskProcess(serverExe, isDevMode) {


So we need async here in order to await during the retry routine at the bottom of this func. But I think async also has the effect of implicitly making this function return a Promise. That's good because it allows await when we call this func. But see what you think about this:

Now, the implicit Promise here resolves to undefined. What if we explicitly resolve with the port number? So main.js would call like const port = await createPythonFlaskProcess(serverExe). And then process.env['PORT'] could be set in main.js instead of inside this function.

I kinda like that because assigning that process.env variable from within the program might be a little unconventional (not that it's necessarily a bad idea). So moving it away from a side-effect of this function and into a more prominent spot in main.js might be a good idea. What do you think?

Oh yea, I do like that way better. Thanks! 2035fa1

davemfish · 2020-12-03T14:46:24Z

src/server.py

+        "--flex", default=True, help="The port number of the Flask Server.")
+    args = vars(parser.parse_args())
+    port = int(args.get('port', 5000))
+    flex = bool(args.get('flex', True))


I'm not sure, are these .get() fallback values redundant with the parser.add_argument default values?

Yep, definitely left over redundancy after iterating. Should be good here: 4c398b9

davemfish · 2020-12-03T14:59:06Z

src/server.py

+        port = port + 1
+
+    # Write to stdout so that parent process can receive the used port number
+    print(f'PORT {port}')


Since we're relying on stdout for sharing data, do you think it makes sense to explicitly call sys.stdout.write ? print definitely does this under the hood by default, but a year from now we might think it's only there for debugging, or something.

Yes! That makes much more sense then a print statement. Here: 4c398b9

This is to help with testing on GHA, as it might take longer for things to spin up.

dcdenu4 · 2020-12-04T21:31:53Z

Hey @davemfish Thanks for the feedback and helping me learn JS stuff. Tests are now passing. I added a retries optional variable to createPythonFlaskProcess, to allow GHA to have more time. I didn't follow all the npx eslint recommendations as some of them weren't very clear and some weren't related to code I changed, so I figured you had thought about, haha.

Let me know what you think!

davemfish

Hey @dcdenu4 , thanks for making those updates! I had some more comments on this round mainly because I'm also still learning how to do things properly in javascript and maybe didn't give great suggestions last time. Also, as we're discussing on the invest PR about the workbench-invest interface, I think it pays to really try to get this right up front.

So there's one main comment about when & how to set the process.env.PORT variable. And another comment about whether we can send data from python to node via a stream other than stdout (so we don't have to check for the special PORT string every time a stdout chunk comes in.

We can talk this stuff over and try to come up with a good solution together!

davemfish · 2020-12-07T16:20:18Z

.env-example


 # Use-case 1. PyInstaller-built invest binaries in some custom location.
-# leave off extensions - the app will add ".exe" if it detects Windows.
+# Add extensions, such as ".exe" if on Windows.


davemfish · 2020-12-07T16:25:15Z

src/main_helpers.js

+    logger.debug(serverExe);
+    pythonServerProcess.stdout.on('data', (data) => {
+      logger.debug(`${data}`);
+      const strData = `${data}`;


minor thing: might as well only format the string once. i.e. logger.debug(strData)

davemfish · 2020-12-07T20:44:10Z

src/main_helpers.js

+        flaskPort = strData.slice(idx + 5, idx + 9);
+        logger.debug(`Flask Server started on Port ${flaskPort}`);
+      }
+    });


I wish I had brought this up on the first round. I'm wondering if there's a cleaner way to get this data from python to nodejs that would avoid checking for the "PORT XXXX" string on every stdout chunk. I think we want to listen on all the stdout chunks so we can log them to the workbench's logger, but we know that the "PORT" string is only coming in once.

I don't have quite enough experience myself to say what the right solution is, but perhaps it's possible to have another stream (apart from stdout) that python sends a message to, and node listens on just once. Maybe we can talk over some options here.

davemfish · 2020-12-08T14:46:19Z

src/main.js

+  }
+  else {
+    logger.error('The resolving of the server port timed out. Try again.');
+  }


Forgive me, I'm still learning javascript too! Last time I suggested returning/resolving the port from createPythonFlaskProcess instead of setting the process.env.PORT as a side-effect of that function.

Looking again, I still think that's a good idea, but I see that it also made things quite a bit more complicated in createPythonFlaskProcess - needing the while loop to wait for a variable assignment before resolving a value.

Now I'm thinking the proper javascript thing to do here could be to pass a callback function to createPythonFlaskProcess that will set process.env.PORT. It would be defined here in main.js but called by the pythonProcess stream listener when the "PORT 5000" string comes through - avoiding the need to "poll" the isPort or flaskPort variable in a while loop before resolving the value.

We can talk this through!

dcdenu4 · 2021-01-04T16:54:32Z

@davemfish Just coming back to this PR after our long break. We had a pretty good long discussion about this and in the end I think we decided to shelf this? I believe we landed on maybe this port handling was over designing and that we should start by sticking with a hard-coded value and see if any issues come up?

davemfish · 2021-01-04T17:12:06Z

@davemfish Just coming back to this PR after our long break. We had a pretty good long discussion about this and in the end I think we decided to shelf this? I believe we landed on maybe this port handling was over designing and that we should start by sticking with a hard-coded value and see if any issues come up?

Yes, that's how I remember it! The stripped down changes that we decided on will go into PRs natcap/invest#399 and #72 . So would love to get your review on those soon, but first I'm checking them over again and I'll post on those PRs when they're ready for review!

dcdenu4 · 2021-01-04T18:49:42Z

Sounds good! I'll close this PR down.

dcdenu4 added 5 commits November 24, 2020 15:32

Rough stab at port automation.

e57f1a8

Handle which port Flask App is started with

0de854a

Fix server port default and invest binary call

c0bc370

Comment out windows conda env setup

08626a7

Deciding to remove commented conda env setup

6acadda

dcdenu4 commented Dec 2, 2020

View reviewed changes

dcdenu4 marked this pull request as ready for review December 2, 2020 19:47

dcdenu4 self-assigned this Dec 2, 2020

dcdenu4 requested a review from davemfish December 2, 2020 20:49

davemfish suggested changes Dec 3, 2020

View reviewed changes

dcdenu4 added 8 commits December 4, 2020 09:16

Update comments and some linting

addc71e

Fix extension handling when in Dev mode

b592fc7

Add listening for port comment and linting

b358eef

Handle setting port env in main.js

2035fa1

stdout write for server port and remove redundancy

4c398b9

Fix int casting and returning promise

c33d7aa

Add await in front of call to fix tests

6879b5d

Add optional arg to createPythonFP to allow retries

d3ffcf9

This is to help with testing on GHA, as it might take longer for things to spin up.

dcdenu4 requested a review from davemfish December 4, 2020 21:31

davemfish suggested changes Dec 8, 2020

View reviewed changes

davemfish mentioned this pull request Dec 10, 2020

Deploy build artifacts to GCS #75

Merged

dcdenu4 closed this Jan 4, 2021

davemfish mentioned this pull request Jan 5, 2021

move python side of workbench to the invest repo #72

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid port conflicts #73

Avoid port conflicts #73

dcdenu4 commented Dec 2, 2020 •

edited

Loading

dcdenu4 Dec 2, 2020

davemfish Dec 3, 2020

dcdenu4 Dec 4, 2020

davemfish left a comment

davemfish Dec 3, 2020

dcdenu4 Dec 4, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

davemfish Dec 3, 2020

dcdenu4 Dec 4, 2020

davemfish Dec 3, 2020

dcdenu4 Dec 4, 2020

davemfish Dec 3, 2020

dcdenu4 Dec 4, 2020

dcdenu4 commented Dec 4, 2020

davemfish left a comment

davemfish Dec 7, 2020

davemfish Dec 7, 2020

davemfish Dec 7, 2020

davemfish Dec 8, 2020

dcdenu4 commented Jan 4, 2021

davemfish commented Jan 4, 2021

dcdenu4 commented Jan 4, 2021

Avoid port conflicts #73

Avoid port conflicts #73

Conversation

dcdenu4 commented Dec 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davemfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcdenu4 commented Dec 4, 2020

davemfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcdenu4 commented Jan 4, 2021

davemfish commented Jan 4, 2021

dcdenu4 commented Jan 4, 2021

dcdenu4 commented Dec 2, 2020 •

edited

Loading