Skip to content

fix: Rewrite transport and queue to prevent memory leaks #1795

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

Jump to bottom
Merged
merged 16 commits into from
Jan 9, 2019
Merged

fix: Rewrite transport and queue to prevent memory leaks #1795

merged 16 commits into from
Jan 9, 2019

Conversation

@kamilogorek
Copy link
Contributor

@kamilogorek kamilogorek commented Dec 12, 2018 •
edited by HazAT
Loading

No description provided.

@kamilogorek kamilogorek requested a review from HazAT December 12, 2018 10:14
@getsentry-bot
Copy link
Contributor

getsentry-bot commented Dec 12, 2018 •
edited
Loading

Fails
🚫

TSLint failed: @sentry/node

  • ERROR: /home/travis/build/getsentry/sentry-javascript/packages/node/src/parsers.ts[110, 11]: Expression is always true.
Messages
📖 @sentry/browser gzip'ed minified size: 22.0762 kB

Generated by 🚫 dangerJS

@HazAT HazAT force-pushed the limit-fs-concurrency branch from 0b37d7a to 8d14c6e Compare December 18, 2018 17:53
@HazAT HazAT changed the title fix: Limit concurrent FS calls to prevent memory fragmentation fix: Rewrite transport and queue to prevent memory leaks Jan 2, 2019
@kamilogorek kamilogorek mentioned this pull request Jan 7, 2019
Merged
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
packages/browser/src/transports/beacon.ts
return {
status: result ? Status.Success : Status.Failed,
};
return this.buffer.add(
Copy link
Contributor Author

@kamilogorek kamilogorek Jan 8, 2019

Choose a reason for hiding this comment

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

Are we sure that we want to store it in the buffer here instead of doing it somewhere else? This way we are forcing users to write this code in all custom transports, otherwise it will skip the buffering completely.

On the other hand, it gives them control over buffering... 🤔

kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
kamilogorek
kamilogorek commented Jan 8, 2019
View reviewed changes
@kamilogorek
Copy link
Contributor Author

kamilogorek commented Jan 8, 2019

👌

@HazAT HazAT merged commit 1d64ace into master Jan 9, 2019
@HazAT HazAT deleted the limit-fs-concurrency branch January 9, 2019 08:25
@rhcarvalho rhcarvalho mentioned this pull request Apr 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HazAT HazAT Awaiting requested review from HazAT

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kamilogorek @getsentry-bot @HazAT