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

Set up Markdown handling exactly once #127

Conversation

savetheclocktower
Copy link
Contributor

@confused-Techie's addition of paranoid logging paid off. One of the instances went sideways again and now we've got a paper trail:

DEFAULT 2024-01-05T09:01:09.914682Z {
DEFAULT 2024-01-05T09:01:09.914704Z error: RangeError: Maximum call stack size exceeded
DEFAULT 2024-01-05T09:01:09.914713Z at String.endsWith (<anonymous>)
DEFAULT 2024-01-05T09:01:09.914719Z at /workspace/src/utils.js:189:94
DEFAULT 2024-01-05T09:01:09.914727Z at Array.find (<anonymous>)
DEFAULT 2024-01-05T09:01:09.914735Z at md.renderer.rules.image (/workspace/src/utils.js:189:61)
DEFAULT 2024-01-05T09:01:09.914742Z at md.renderer.rules.image (/workspace/src/utils.js:196:14)
DEFAULT 2024-01-05T09:01:09.914749Z at md.renderer.rules.image (/workspace/src/utils.js:196:14)
DEFAULT 2024-01-05T09:01:09.914756Z at md.renderer.rules.image (/workspace/src/utils.js:196:14)
DEFAULT 2024-01-05T09:01:09.914763Z at md.renderer.rules.image (/workspace/src/utils.js:196:14)
DEFAULT 2024-01-05T09:01:09.914770Z at md.renderer.rules.image (/workspace/src/utils.js:196:14)
DEFAULT 2024-01-05T09:01:09.914795Z at md.renderer.rules.image (/workspace/src/utils.js:196:14),
DEFAULT 2024-01-05T09:01:09.914801Z dev: false,
DEFAULT 2024-01-05T09:01:09.914807Z timecop: false,
DEFAULT 2024-01-05T09:01:09.914813Z page: {
DEFAULT 2024-01-05T09:01:09.914827Z name: 'PPR Error Page',
DEFAULT 2024-01-05T09:01:09.914835Z og_url: 'https://web.pulsar-edit.dev/packages',
DEFAULT 2024-01-05T09:01:09.914842Z og_description: 'The Pulsar Package Repository',
DEFAULT 2024-01-05T09:01:09.914848Z og_image: 'https://web.pulsar-edit.dev/public/pulsar_name.svg',
DEFAULT 2024-01-05T09:01:09.914854Z og_image_type: 'image/svg+xml'
DEFAULT 2024-01-05T09:01:09.914860Z },
DEFAULT 2024-01-05T09:01:09.914865Z status_to_display: false
DEFAULT 2024-01-05T09:01:09.914872Z }

When we render a package README, we do some custom processing of URLs because we want images and links to point to the right places instead of 404ing. In some cases, this custom processing needs access to metadata about the current package, so we were adding new callbacks inside of the prepareForDetail function in utils.js. But each time we added a new handler, we were calling the old one at the end… so we were building up an onion skin of link handlers.

Eventually, after an instance handles one too many requests to view a package detail page, an attempt to rewrite a single image URL fails with Maximum call stack size exceeded.

The fix here is to ensure that we add our custom callbacks only once. To get around the issue of needing access to package information, we can declare let currentPackage in the top-level scope, then assign it to our new pack object only for as long as needed to generate README markup. Definitely a hack, but right now I don't care.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Looks fantastic!
This is an amazing find, and seems to totally be the root cause of the issue.

Read over the code and the changes made here are only what's intended, even if the GitHub diff is a little awkward.

Additionally, I've gone ahead and tested that everything still works exactly as expected, which awesomely, it still does.

Finally I did want to test to see if there was any memory usage difference. While very unscientific, running locally after about 3 dozen refreshes on your branch, the memory usage stayed about the same, with the heap being used never higher than 34 MB, meanwhile on master branch after about 3 dozen refreshes we can watch the heap used steadily increasing to about 50 MB. So there is a very obvious difference here, and beyond the logical conclusion this is the issue, it's awesome to see that in practice as well.

But in any case, lets get this one merged, thanks a ton for this awesome find, and the PR to boot!

@confused-Techie confused-Techie merged commit 04c31e8 into pulsar-edit:main Jan 5, 2024
2 checks passed
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.

2 participants