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

Fix reading source file multiple times #2569

Merged
merged 2 commits into from
May 11, 2020
Merged

Fix reading source file multiple times #2569

merged 2 commits into from
May 11, 2020

Conversation

pushplay
Copy link
Contributor

@pushplay pushplay commented May 5, 2020

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@sentry/node/parsers/readSourceFiles() expects the list of source files to be deduplicated (

// we're relying on filenames being de-duped already
). If parseStack() doesn't deduplicate the list then the same file is opened multiple times. If the file is large (which can happen when using webpack) then this can waste enough memory to be a problem on a memory-constrained instance.

For example this stack:

[ { columnNumber: 11,
    fileName: '/var/task/index.js',
    functionName: null,
    lineNumber: 339944,
    methodName: null,
    native: false,
    typeName: 'Object' },
  { columnNumber: null,
    fileName: null,
    functionName: 'Generator.next',
    lineNumber: null,
    methodName: 'next',
    native: false,
    typeName: 'Generator' },
  { columnNumber: 67,
    fileName: '/var/task/index.js',
    functionName: null,
    lineNumber: 339920,
    methodName: null,
    native: false,
    typeName: null },
  { columnNumber: null,
    fileName: null,
    functionName: 'new Promise',
    lineNumber: null,
    methodName: null,
    native: false,
    typeName: null },
  { columnNumber: 10,
    fileName: '/var/task/index.js',
    functionName: 'module.exports../src/lambdas/rest/user/index.ts.__awaiter',
    lineNumber: 339899,
    methodName: '__awaiter',
    native: false,
    typeName: 'module.exports../src/lambdas/rest/user/index.ts' },
  { columnNumber: 47,
    fileName: '/var/task/index.js',
    functionName: 'Object.router.route.handler.evt [as handler]',
    lineNumber: 339943,
    methodName: 'evt [as handler]',
    native: false,
    typeName: 'Object.router.route.handler' },
  { columnNumber: 42,
    fileName: '/var/task/index.js',
    functionName: null,
    lineNumber: 13489,
    methodName: null,
    native: false,
    typeName: 'BuildableRoute' },
  { columnNumber: null,
    fileName: null,
    functionName: 'Generator.next',
    lineNumber: null,
    methodName: 'next',
    native: false,
    typeName: 'Generator' },
  { columnNumber: 67,
    fileName: '/var/task/index.js',
    functionName: null,
    lineNumber: 13429,
    methodName: null,
    native: false,
    typeName: null },
  { columnNumber: null,
    fileName: null,
    functionName: 'new Promise',
    lineNumber: null,
    methodName: null,
    native: false,
    typeName: null },
  { columnNumber: 10,
    fileName: '/var/task/index.js',
    functionName:
     'module.exports../node_modules/cassava/dist/routes/BuildableRoute.js.__awaiter',
    lineNumber: 13408,
    methodName: '__awaiter',
    native: false,
    typeName:
     'module.exports../node_modules/cassava/dist/routes/BuildableRoute.js' },
  { columnNumber: 12,
    fileName: '/var/task/index.js',
    functionName: 'BuildableRoute.handle',
    lineNumber: 13471,
    methodName: 'handle',
    native: false,
    typeName: 'BuildableRoute' },
  { columnNumber: 34,
    fileName: '/var/task/index.js',
    functionName: null,
    lineNumber: 12653,
    methodName: null,
    native: false,
    typeName: 'Router' },
  { columnNumber: null,
    fileName: null,
    functionName: 'Generator.next',
    lineNumber: null,
    methodName: 'next',
    native: false,
    typeName: 'Generator' },
  { columnNumber: 24,
    fileName: '/var/task/index.js',
    functionName: 'fulfilled',
    lineNumber: 12516,
    methodName: null,
    native: false,
    typeName: null },
  { columnNumber: 7,
    fileName: 'internal/process/next_tick.js',
    functionName: 'process._tickCallback',
    lineNumber: 68,
    methodName: '_tickCallback',
    native: false,
    typeName: 'process' } ]

generates filesToRead

[ '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js',
  '/var/task/index.js' ]

@pushplay pushplay requested a review from kamilogorek as a code owner May 5, 2020 17:35
@pushplay
Copy link
Contributor Author

pushplay commented May 5, 2020

The build is stuck on 1995.6 @sentry/browser - browserstack integration tests

$ node scripts/checkbrowsers.js
Unable to find API credentials in env. Please export them as BROWSERSTACK_USERNAME and BROWSERSTACK_ACCESS_KEY.

Is that something I'm responsible for?

Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

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

No, you are not responsible for the fail, all good.
Thank you for the PR!

I wonder why this happened, the reason is this:

https://github.com/getsentry/sentry-javascript/pull/2569/files#diff-fce4b2217c221b6d7f9e46155ca24471R88

We have a cache for the file content if we already read the file, I wonder if this is not working.
But regardless I think the change is fine anyway :)

@pushplay
Copy link
Contributor Author

pushplay commented May 6, 2020

We have a cache for the file content if we already read the file, I wonder if this is not working.

There is a cache but it's only written to once the file is read. So the cache can only be hit on consecutive calls to readSourceFiles.

Copy link
Contributor

@rhcarvalho rhcarvalho left a comment

Choose a reason for hiding this comment

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

Works for me.

Wondering if filesToRead should have been a Set.

@kamilogorek kamilogorek merged commit 0dd8962 into getsentry:master May 11, 2020
@kamilogorek
Copy link
Contributor

@rhcarvalho no, as we cannot use Set due to IE10-IE11 support. They don't have Set API.

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

Successfully merging this pull request may close these issues.

None yet

4 participants