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

bug: Copy task runs in parallel to build tasks creating a race condition #5592

Open
3 tasks done
mt3o opened this issue Mar 26, 2024 · 9 comments
Open
3 tasks done

bug: Copy task runs in parallel to build tasks creating a race condition #5592

mt3o opened this issue Mar 26, 2024 · 9 comments
Assignees
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil

Comments

@mt3o
Copy link

mt3o commented Mar 26, 2024

Prerequisites

Stencil Version

4.13.0

Current Behavior

When building with stencil, when the copy task is issued here:
https://github.com/ionic-team/stencil/blob/main/src/compiler/output-targets/index.ts#L30

there is Promise.all calling all tasks, including copy tasks, and regular building tasks. When the build (compilation, regular targets) takes longer, copy task is executed first, trying to copy non-existing directories. It's a race condition between compilation and copying.

I have problems replicating the issue with a smaller repo, and obviously, I can't share my work project. I'm guessing the issue happens only in big projects, or with poor hardware, as it's a race it depends on the performance.

For me - the workaround is to either create empty directories first (risking incomplete build), or not use copy-task at all, and instead copy files in standalone CI/CD script.
On developer machine, with watch mode, the problem is nearly nonexistent, as there are some files in /dist all the time. If something doesn't work, devs just restart the build script and then it goes smoothly. Such behavior in CI/CD is difficult to achieve.

Expected Behavior

Please consider moving await outputCopy(config, compilerCtx, buildCtx), after the Promise.all and before the www target. This should solve the race problem.

System Info

System: node 18.16.1
    Platform: windows (10.0.19045)
   CPU Model: 11th Gen Intel(R) Core(TM) i7-11850H @ 2.50GHz (16 cpus)
    Compiler: C:\foobar\node_modules\@stencil\core\compiler\stencil.js
       Build: 1710170895
     Stencil: 4.12.6
  TypeScript: 5.3.3
      Rollup: 2.56.3
      Parse5: 7.1.2
      jQuery: 4.0.0-pre
      Terser: 5.29.1

Steps to Reproduce

Set up a big repo with complex components,
Add lots of targets to be compiled
Set www target to be built
Have all targets be copied into www
run npx stencil build

Stencil runs all tasks as in Promise.all call, copy ends after build tasks are over, compilation breaks with error that some dirs don't exist in /dist.

Code Reproduction URL

no url :(

Additional Information

The log is as follows, it's that the clear copy task is ending before the build tasks are finished. So I'm concluding that moving the copy task out of Promise.all will solve the problem - because the copy task will be executed only after all build tasks are done.

[58:20.3]  create workers, maxWorkers: 7  MEM: 142.1MB
[58:20.3]  Starting compilation in watch mode...  MEM: 142.1MB
[58:24.3]  build, search, dev mode, started ...
[58:24.3]  start build, 2024-03-26T17:58:24  MEM: 453.9MB
[58:24.3]  cleaning 6 dirs ...  MEM: 453.9MB
[58:24.3]  cleaning dirs finished in 2 ms  MEM: 454.0MB
[58:24.4]  transpile started ...
[58:32.5]  Transpiled modules: [ ... ]

[58:32.5]  generated app types started ...  MEM: 744.0MB
[58:32.5]  generated app types finished: src/components.d.ts in 22 ms  MEM: 747.4MB
[58:32.5]  transpile finished in 8.18 s
[58:32.5]  generate outputs started ...  MEM: 747.6MB
[58:32.5]  getComponentAssetsCopyTasks: 8  MEM: 747.6MB
[58:32.5]  getComponentAssetsCopyTasks: 8  MEM: 747.6MB
[58:32.5]  copy started ...
[58:32.5]  generate hydrate app started ...
[58:32.5]  generate lazy + source maps started ...
[58:32.5]  generateEntryModules, 144 entryModules  MEM: 747.7MB
[58:36.4]  copy finished (862 files) in 3.84 s
[59:02.7]  generate hydrate app finished in 30.14 s
[59:06.9]  generate lazy + source maps finished in 34.35 s
[59:06.9]  generate www started ...  MEM: 2443.5MB
[59:06.9]  generateIndexHtml, write: www/index.html  MEM: 2443.5MB
[59:06.9]  generate www finished in 6 ms  MEM: 2443.5MB
[59:07.1]  updated readme docs: (...)
[59:07.1]  updated readme docs: (...)
[59:07.1]  updated readme docs: (...)
[59:07.1]  generate types started ...  MEM: 2451.7MB
[59:07.1]  generate types finished in 1 ms  MEM: 2451.7MB
[59:07.1]  generate outputs finished in 34.55 s  MEM: 2451.7MB
[59:07.1]  aborted build, 42750ms  MEM: 2451.8MB

[ ERROR ]  ENOENT: no such file or directory, stat 'C:\foobar\dist\lazy-loader'

[ ERROR ]  ENOENT: no such file or directory, stat 'C:\foobar\dist\custom-elements'

[ ERROR ]  ENOENT: no such file or directory, stat 'C:\foobar\dist\types'

[ ERROR ]  ENOENT: no such file or directory, stat 'C:\foobar\dist\search'
@ionitron-bot ionitron-bot bot added the triage label Mar 26, 2024
@rwaskiewicz rwaskiewicz self-assigned this Mar 26, 2024
@rwaskiewicz
Copy link
Member

Hey @mt3o 👋

While I can see how this could be an issue, some of these reproduction steps are too vague for us to be able to move forward with this. While we don't want you to share any work you're not permitted to, we do need some type of reproduction case here to clarify things like:

  1. Output targets used
  2. How each output target is configured

Otherwise, we could spend quite a bit of time building something that doesn't actually reproduce the problem you're seeing here.

One possibility, would be to use the stencil component starter (npm init stencil@latest component) to build a component library, installing its dependencies, then running the generate command (npm run generate) build out a bunch of components here.

Another would be to use the Ionic Framework (built with Stencil) and try to use it and its stencil config to try to reproduce the problem.

@rwaskiewicz rwaskiewicz added the ionitron: needs reproduction This PR or Issue does not have a reproduction case URL label Mar 26, 2024
@rwaskiewicz rwaskiewicz removed their assignment Mar 26, 2024
Copy link

ionitron-bot bot commented Mar 26, 2024

Thanks for the issue! This issue has been labeled as needs reproduction. This label is added to issues that need a code reproduction.

Please reproduce this issue in an Stencil starter component library and provide a way for us to access it (GitHub repo, StackBlitz, etc). Without a reliable code reproduction, it is unlikely we will be able to resolve the issue, leading to it being closed.

If you have already provided a code snippet and are seeing this message, it is likely that the code snippet was not enough for our team to reproduce the issue.

For a guide on how to create a good reproduction, see our Contributing Guide.

@ionitron-bot ionitron-bot bot removed the triage label Mar 26, 2024
@mt3o
Copy link
Author

mt3o commented Mar 27, 2024

Hi
Unfortunately i'm unable to reproduce the issue in a fresh small repo. Race conditions are usually difficult to reproduce ;-)
I will try again later today, scripting to create more components.
But I can share the config.

import {sass} from '@stencil/sass';
import type {Config} from '@stencil/core';
import {inlineSvg} from 'stencil-inline-svg';
import { rollapVisualizerPluginConfig } from './project-configuration-extensions/project-configuration-extensions-configs';
import { configurationExtensionsLoader } from './project-configuration-extensions/project-configuration-extensions-loader';
import {getGitHash} from './src/getGitHash'

const parseBoolean = (value: string|undefined, fallback: boolean) => {
  if(value===undefined) return fallback;
  if (['true','1','on','enabled'].includes(value.toLowerCase())) return true;
  if (['false','0','off','disabled'].includes(value.toLowerCase())) return false;
  return fallback;
}

// *my workaround for the race condition, on ci/cd its set to true*
const stencilNoCopy = parseBoolean(process.env.STENCIL_NO_COPY, false)

const copyDist = stencilNoCopy ? [] : [
  // {src: '../dist/esm', dest: 'esm'},
  {src: '../dist/lazy-loader', dest: 'lazy-loader'},
  {src: '../dist/custom-elements', dest: 'custom-elements'},
  {src: '../dist/ui', dest: 'ui'},
  {src: '../dist/types', dest: 'types'},
]
const copy = [
  ...copyDist,
  {src: '../node_modules/@xx/fonts/css', dest: 'fonts/css'},
  {src: '../node_modules/@xx/fonts/fonts', dest: 'fonts/fonts'},
  {src: '../node_modules/@xx/icons/css', dest: 'icons/css'},
  {src: '../node_modules/@xx/icons/fonts', dest: 'icons/fonts'},
  {src: '../node_modules/@xx/web-ui', dest: 'web-ui'},
  {src: 'demo-pages', dest: 'demo-pages'},
];


// noinspection JSUnusedGlobalSymbols
export const config: Config = {
  namespace: 'ui',
  devServer: {
    openBrowser: false,
  },
  autoprefixCss: true,
  taskQueue: 'async',
  minifyJs: parseBoolean(process.env['MINIFY_JS'],true),
  minifyCss: parseBoolean(process.env['MINIFY_CSS'],true),
  excludeUnusedDependencies: true,
  extras: {
    appendChildSlotFix: true,
    cloneNodeFix: true,
    enableImportInjection: true,
  },
  validateTypes: true,
  validatePrimaryPackageOutputTarget: true,


  hashFileNames: false,
  outputTargets: [
    {
      type: 'dist',
      esmLoaderPath: 'loader',
    },
    {
      type: "dist-custom-elements",
      customElementsExportBehavior: 'bundle', 

      isPrimaryPackageOutputTarget: true,
      minify: true,
      dir: 'dist/custom-elements',
    },
    {
      type: 'stats',
    },
    {
      type: 'dist-lazy',
      dir: 'dist/lazy-loader',
      cjsDir:'dist-lazy-loader-cjs',
      esmDir: 'dist-lazy-loader-esm',
      empty: true,
    },
    {
      type: 'docs-readme',
    },
    {
      type: 'docs-json',
      file: 'documentation.json'
    },
    {
      type: 'docs-vscode',
      file: 'custom-elements.json',
    },
    {
      type: 'dist-hydrate-script',
    },
    {
      type: 'www',
      baseUrl: 'https://www.se.com/',
      copy,
      serviceWorker: null, // disable service workers
    },
  ],
  testing: {
    verbose: true,
    

    browserArgs: ['--no-sandbox', '--disable-setuid-sandbox'],
    transform: {
      '^.+\\.svg$': '<rootDir>/svgTransform.js',
    },
    transformIgnorePatterns: ['/node_modules/(?!@xx/icons.svg)'],
    roots: ['<rootDir>'],
    collectCoverage: true,

    //uncomment if you want to see the browser during tests
    // browserHeadless: false,
    // browserDevtools: true,
    // browserSlowMo: 1000, //milliseconds
  },
  preamble: 'UI script \ncompilation date: ' + Date()+'\ncommit: '+ getGitHash(),
  plugins: [
    sass({
      injectGlobalPaths: [
        'node_modules/@xx/web-ui/styles/_theme.scss',
        './src/styles/global.scss',
        './src/styles/variables.scss',
        './src/styles/mixins/index.scss',
      ],
    }),
    inlineSvg(),
    ...configurationExtensionsLoader(rollapVisualizerPluginConfig)
  ],
};

@mt3o
Copy link
Author

mt3o commented Apr 23, 2024

I finally created the repo to present the problem so that you can replicate.

https://github.com/mt3o/stencil-race-condition-between-copy-and-build

https://stackblitz.com/~/github.com/mt3o/stencil-race-condition-between-copy-and-build

Please verify

@rwaskiewicz rwaskiewicz added triage and removed ionitron: needs reproduction This PR or Issue does not have a reproduction case URL labels Apr 25, 2024
@rwaskiewicz rwaskiewicz self-assigned this May 1, 2024
@rwaskiewicz
Copy link
Member

Hey @mt3o,

Thanks for the repro! Can you provide instructions for using it to verify this bug? I see a generate-scripts.mjs file in this directory, is that supposed to be be used in some way? Should we run the build script before/after this is run (if at all)?

Thanks!

@rwaskiewicz rwaskiewicz added the Awaiting Reply This PR or Issue needs a reply from the original reporter. label May 1, 2024
@ionitron-bot ionitron-bot bot removed the triage label May 1, 2024
@mt3o
Copy link
Author

mt3o commented May 23, 2024

No, you don't have to do anything. Just npm install && npm start. I left the generate-scripts.mjs file so that the experiment could be recreated, and more files could be created to make the stencil compilation to take even longer.

If you open StackBlitz link you should see this:
image

It fails, because it tries to copy files that don't exist. Yet.

Please note lines 22, 23, 24 in stencil.config.ts where there is the task to the not-yet-existing copy files. Files that are supposed to be built during the compilation. The npm start failed because the copy task expects compilation output to be present, and fails because it's not there. This is the race condition I tried to explain in the beginning. Copy happens faster than the compilation, so if you copy the file that is to be created during the compilation - the operation fails.

If you comment out these lines, disabling the copying, running npm start ends with success. Avoiding the attempt to copy the compilation output - allows the compilation to be processed successfully.

image

What's interesting, if you uncomment those lines, and rerun the compilation - it ends with success. The reason is simple - now past compilation artifacts exist, can be copied - so there is no error, and the compilation eventually gets done. What's annoying, if you - the frontend developer - alter your code, first attempt to build - will reuse your old files. Rerunning the build process will copy proper files.

@ionitron-bot ionitron-bot bot removed the Awaiting Reply This PR or Issue needs a reply from the original reporter. label May 23, 2024
@christian-bromann christian-bromann added the Bug: Validated This PR or Issue is verified to be a bug within Stencil label May 24, 2024
@christian-bromann
Copy link
Member

@mt3o thanks, I've ingested this issue into our backlog. At this point I can't provide any estimate as to when we get the chance to look into it. We appreciate any help on the issue and are available for questions as they arise.

@mt3o
Copy link
Author

mt3o commented May 28, 2024

@christian-bromann

The fix would be, in file https://github.com/ionic-team/stencil/blob/main/src/compiler/output-targets/index.ts#L30

to alter the block:

  await Promise.all([
    //outputCopy(config, compilerCtx, buildCtx), //remove this one
    outputCollection(config, compilerCtx, buildCtx, changedModuleFiles),
    outputCustomElements(config, compilerCtx, buildCtx),
    outputHydrateScript(config, compilerCtx, buildCtx),
    outputLazyLoader(config, compilerCtx),
    outputLazy(config, compilerCtx, buildCtx),
  ]);

  //and call it after compilation is done here, so that all files are already in place
  await outputCopy(config, compilerCtx, buildCtx) 

  await outputWww(config, compilerCtx, buildCtx);

@christian-bromann
Copy link
Member

@mt3o thanks, mind raising a PR for this? The team would greatly appreciate any help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug: Validated This PR or Issue is verified to be a bug within Stencil
Projects
None yet
Development

No branches or pull requests

3 participants