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

promisify crashing node #525

Closed
SamMousa opened this issue Sep 11, 2023 · 6 comments · Fixed by #519
Closed

promisify crashing node #525

SamMousa opened this issue Sep 11, 2023 · 6 comments · Fixed by #519
Labels

Comments

@SamMousa
Copy link

I have several different packages where semantic release is no longer working.
The last line in the log is this:

[3:27:11 PM] [semantic-release] › ℹ  Start step "analyzeCommits" of plugin "@semantic-release/commit-analyzer"

The exit code is 0 so my pipelines proceed but no release is created.
This happens on several repositories, most open source, I'll link some CI traces here:

The code causing this issue lives in load-parser-config.js:

export default async ({ preset, config, parserOpts, presetConfig }, { cwd, logger }) => {
  let loadedConfig;
  const __dirname = dirname(fileURLToPath(import.meta.url));

  if (preset) {
    const presetPackage = `conventional-changelog-${preset.toLowerCase()}`;
    loadedConfig = importFrom(cwd, presetPackage);
  } else if (config) {
    loadedConfig = importFrom.silent(__dirname, config) || importFrom(cwd, config);
  } else {
    loadedConfig = conventionalChangelogAngular;
  }

  // Comments added by @sammousa. 
  // loadedConfig is an async function, specifically the createPreset export from conventional-changelog-conventionalcommits package
  // which implements the conventional commit standard that this package uses
  loadedConfig = await (typeof loadedConfig === "function"
      ? isPlainObject(presetConfig)
          ? loadedConfig(presetConfig)
          // this line crashes nodejs silently when called with an async function
          : promisify(loadedConfig)()
      : loadedConfig);

  return { ...loadedConfig.parserOpts, ...parserOpts };
};

The bug is definitely in nodeJS, the fix for this package however would be to not use promisify at all, since we're calling the function directly if there IS a configuration, why not also call it directly when there isn't?

@travi
Copy link
Member

travi commented Sep 11, 2023

looks like you are running into a combination of semantic-release/semantic-release#2929 and semantic-release/semantic-release#2932. we have PRs started in semantic-release/release-notes-generator#526 and #519 to address semantic-release/semantic-release#2929. in https://github.com/semantic-release/release-notes-generator/pull/526/files#diff-bee027b39eb704f3c940d54960f4f26693260c52d72707ac17d72f38f66da7d5L35-L40, we've removed that usage of promisify and plan to do the same in this package as well.

thanks for the investigation into the symptoms. i think we are covered this specific fix already in the beta, but interested in more feedback if you find more that we should investigate.

please give the beta version a try, as mentioned in semantic-release/semantic-release#2929, and let us know if you still have any problems with that.

@SamMousa
Copy link
Author

For now my workaround is just to specify an empty presetConfig.
I'm confused why i'm seeing exit code 0 though; since node itself should be dying with exit code 13...

@travi
Copy link
Member

travi commented Sep 11, 2023

that is the reason we've created semantic-release/semantic-release#2932 we've had other reports of similar exiting with the wrong exit code for other failures. it is likely that we are not catching something properly somewhere. i think the key detail is that it is not specific to this issue, but is noticed when other legitimate failures occur. we agree that this is a problem, but have been mostly focused on semantic-release/semantic-release#2929 up to this point.

if you would be interested in helping us investigate this problem, we would greatly appreciate the help.

@rvagg
Copy link

rvagg commented Sep 12, 2023

It's because of this: conventional-changelog/conventional-changelog#1045

conventional-changelog are now exporting async functions rather than functions that optionally accept a callback, so promisify() is silently failing, thinking it's done its work having not received anything back on the callback .. because there is none. So no errors, no exit code!=0.

This fixes it in lib/load-parser-config.js, but it's not pretty:

  loadedConfig = await (typeof loadedConfig === "function"
    ? isPlainObject(presetConfig)
      ? loadedConfig(presetConfig)
      : loadedConfig.constructor.name === 'AsyncFunction'
        ? loadedConfig()
        : promisify(loadedConfig)()
    : loadedConfig);

For me, the workaround is to pin to conventional-changelog-conventionalcommits@6 because 7.0.0 is where the change is introduced: https://github.com/conventional-changelog/conventional-changelog/releases/tag/conventional-changelog-conventionalcommits-v7.0.0

My bad for not pinning versions properly in my hacky Github Actions config.

@travi
Copy link
Member

travi commented Sep 12, 2023

It's because of this: conventional-changelog/conventional-changelog#1045

thanks for the additional investigation. good to hear that this particular situation is likely to be covered by our pending changes.

we're working on getting a beta version promoted to stable to handle the updates from conventional-changelog, which should handle this and remove the use of promisify. that beta can be found at semantic-release/semantic-release#2934. once we get that out, we still plan to investigate improper exits since we have had some other reports before this conventional-changelog update, so i think we still have some additional situations to handle.

@github-actions
Copy link

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dib542 added a commit to duality-labs/duality-web-app that referenced this issue Sep 20, 2023
dib542 added a commit to duality-labs/duality-web-app that referenced this issue Sep 20, 2023
* feat: update to semantic-release-action@v4 (using Node 20)

    - also upgrades semantic version to 21.0.9
    - link: cycjimmy/semantic-release-action@0c20554

* feat: test out dry_run

* feat: add presetConfig to commit-analyzer step

    - which is noted as being a required field
    - link: https://github.com/semantic-release/commit-analyzer/tree/v11.0.0#options

* fix: presetConfig not needed on previous conventionalcommits versions

    - noted by: semantic-release/commit-analyzer#525

* feat: pin all dependencies to known versions that work

* Revert "feat: test out dry_run"

This reverts commit e8d8e73.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants