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.

Sign up for GitHub

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

Upgrade node v14 to v16 #7081

Merged
merged 1 commit into from
Sep 30, 2022
Merged

Upgrade node v14 to v16 #7081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
14.18.2
16.14.0
4 changes: 2 additions & 2 deletions src/deploy/NVA_build/noobaa_init.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,14 @@ init_noobaa_agent() {

cd /root/node_modules/noobaa-core/
prepare_agent_conf
run_internal_process node ./src/agent/agent_cli
run_internal_process node --unhandled-rejections=warn ./src/agent/agent_cli
Copy link
Member

Choose a reason for hiding this comment

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

@Utkarsh-pro Did you consider setting it in the NODE_OPTIONS env instead of adding to all calls?
See https://nodejs.org/docs/latest-v16.x/api/cli.html#node_optionsoptions
But if we are going to clean up these warnings soon, as we should, then it doesn't matter anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@guymguym, actually I did open up an issue for cleaning this up here: #7083. But as I mentioned in the issue, I think we would need a tiny bit more help from the runtime as right now it doesn't event shows that the uncaughtException was rooted from unhandledRejection and has no traces of it in the stack trace either. That's why resorted to this.

I think I completely missed considering the node options though, thank you!

}

migrate_dbs() {
fix_non_root_user

cd /root/node_modules/noobaa-core/
/usr/local/bin/node src/upgrade/migration_to_postgres.js
/usr/local/bin/node --unhandled-rejections=warn src/upgrade/migration_to_postgres.js
}

if [ "${RUN_INIT}" == "agent" ]
Expand Down
6 changes: 3 additions & 3 deletions src/deploy/NVA_build/noobaa_supervisor.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ stdout_logfile=/dev/fd/1
stdout_logfile_maxbytes=0
stderr_logfile=/dev/fd/1
stderr_logfile_maxbytes=0
command=/usr/local/bin/node %(ENV_WEB_NODE_OPTIONS)s src/server/web_server.js
command=/usr/local/bin/node %(ENV_WEB_NODE_OPTIONS)s --unhandled-rejections=warn src/server/web_server.js
#endprogram

[program:bg_workers]
Expand All @@ -21,7 +21,7 @@ stdout_logfile=/dev/fd/1
stdout_logfile_maxbytes=0
stderr_logfile=/dev/fd/1
stderr_logfile_maxbytes=0
command=/usr/local/bin/node %(ENV_BG_NODE_OPTIONS)s src/server/bg_workers.js
command=/usr/local/bin/node %(ENV_BG_NODE_OPTIONS)s --unhandled-rejections=warn src/server/bg_workers.js
#endprogram

[program:hosted_agents]
Expand All @@ -34,7 +34,7 @@ stdout_logfile=/dev/fd/1
stdout_logfile_maxbytes=0
stderr_logfile=/dev/fd/1
stderr_logfile_maxbytes=0
command=/usr/local/bin/node %(ENV_HOSTED_AGENTS_NODE_OPTIONS)s src/hosted_agents/hosted_agents_starter.js
command=/usr/local/bin/node %(ENV_HOSTED_AGENTS_NODE_OPTIONS)s --unhandled-rejections=warn src/hosted_agents/hosted_agents_starter.js
#endprogram

[program:rsyslog]
Expand Down
3 changes: 2 additions & 1 deletion src/util/chunk_fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,8 @@ class ChunkFS extends stream.Transform {
}

async _flush(callback) {
this._flush_buffers(callback);
// wait before the last writev to finish
await this._flush_buffers(callback);
Comment on lines +62 to +63
Copy link
Member

Choose a reason for hiding this comment

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

Hey @Utkarsh-pro
_flush() is not really an async function in nodejs streams, and the caller will not await for its promise, so it's a bit misleading. It passes a callback function instead - which we handle correctly inside _flush_buffers. The fact that we defined _flush() itself to be async was accidental. So if you get complaints on unhandled rejection of the promise returned by _flush_buffers() I would instead add a noop catch handler to it like _flush_buffers(callback).catch(noop) - you can see how we do it in other places -

noobaa-core/src/server/system_services/system_store.js

Line 374 in cfbf726

.catch(_.noop);

noobaa-core/src/rpc/ice.js

Line 1323 in 7a31c5d

self.defer.promise.catch(_.noop); // to ignore 'Unhandled rejection' printouts

noobaa-core/src/util/postgres_client.js

Line 1466 in db1bc28

seq.init_promise = seq._create(this.pool).catch(_.noop); // TODO what is best to do when init_collection fails here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@guymguym, actually that's true but for recent node versions. In node v16, although undocumented, it does calls then if a promise is returned. https://github.com/nodejs/node/blob/418ff70b810f0e7112d48baaa72932a56cfa213b/lib/internal/streams/transform.js#L131

Here's my understanding code, please correct me if this is wrong:

  1. We have async _flush (accidental probably). This function always returns a promise.
  2. Node sees that the returned value has .then attached to it.
  3. As soon as the promise is resolved (which without await on our flush_buffer resolves almost immediately).
  4. This leads to call this.push(null) in the node source.
  5. Call to this.push(null) leads to emitting of readable event.
  6. That leads to calling next which realises that oops, we have read everything.
  7. Emits end event.
  8. finished() already encountered the finish event by this time, only thing holding it back from getting resolved was the end event from the transform stream.
  9. Above emit of end event caused the finished() to resolve.

And that's how I think we exit the finished before we intended. As mentioned, this causes the test to non-deterministic, sometimes promise resolves first (when read buffer indeed requires flushing) and sometimes callback is invoked first (desired).

Copy link
Member Author

@tangledbytes tangledbytes Oct 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a sample which won't work in node v16.14.0 (but would in recent and in v14):

const util = require("util");
const timeout = util.promisify(setTimeout);
const crypto = require("crypto");
const { Readable, Transform, pipeline, finished } = require("stream");
const pipelineAsync = util.promisify(pipeline);
const finishedAsync = util.promisify(finished);

async function P(...streams) {
  return pipelineAsync(...streams).then(() => {
    streams[streams.length - 1].resume();
  });
}

class TargetHash {
  constructor() {
    this.hash = crypto.createHash("md5");
  }

  digest() {
    return this.hash.digest("hex");
  }

  async writev(_config, buffers) {
    await timeout(500);
    for (const buf of buffers) this.hash.update(buf);
  }
}

class T extends Transform {
  constructor({ targetHash }) {
    super();
    this.th = targetHash;
  }

  async _transform(chunk, encoding, callback) {
    await this.th.writev({}, [chunk]);
    callback();
  }

  async _flush(callback) {
    console.log("flush start");
    this._flush_work(callback);
    console.log("flush end");
  }

  async _flush_work(cb) {
    console.log("flush_work start");
    await this.th.writev({}, [Buffer.from("EOF")]);
    console.log("flush_work end");

    console.log("flush_work cb start");
    const res = cb();
    console.log("flush_work cb end");
    return res;
  }
}

async function main() {
  const th = new TargetHash();
  const t = new T(
    { targetHash: th },
  );
  const r = Readable.from(
    (async function* () {
      for (let i = 0; i < 1e1; i++) {
        yield i.toString();
      }
    })()
  );

  console.log("pipeline start");
  await P(r, t);
  await finishedAsync(t);
  console.log("DIGEST:", th.digest());
  console.log("pipeline finished");
}

main();

}

// callback will be passed only at the end of the stream by _flush()
Expand Down