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

Process crash while SitemapAndIndexStream without any map item providered #392

Open
tianyingchun opened this issue May 15, 2022 · 4 comments

Comments

@tianyingchun
Copy link

Describe the bug
Process crash while SitemapAndIndexStream without any map item providered

Expected behavior

work correctly , should not crash the program process.

Context:
"sitemap": "^7.1.1"

Additional context

const sms = new SitemapAndIndexStream({
      limit: jobSiteConfig.limit || 10000, // defaults to 45k
      // SitemapAndIndexStream will call this user provided function every time
      // it needs to create a new sitemap file. You merely need to return a stream
      // for it to write the sitemap urls to and the expected url where that sitemap will be hosted
      // https://github.com/ekalinin/sitemap.js/blob/master/examples/sitemapAndIndex.js
      getSitemapStream: (i) => {
        const sitemapStream = new SitemapStream({
          hostname: jobSiteConfig.hostname,
        });
        const path = `./${this.sitemapDir}/${siteName}/sitemap-${i}.xml`;
        const ws = createWriteStream(resolve(path));
        sitemapStream.pipe(ws); // write it to sitemap-NUMBER.xml
        return [new URL(path, sitemapHost).toString(), sitemapStream, ws];
      },
    })

    sms.pipe(createWriteStream(resolve(`./${this.sitemapDir}/${siteName}/sitemap-index.xml`)));

// Note sitemapItems == [], it will throw `[EmptySitemap: You ended the stream without writing anything.]`
    for (const item of sitemapItems) {
      sms.write(item);
    }

    sms.end()
@tianyingchun
Copy link
Author

should provider us catch the error, if we don't provider any sitemap item, it will directly crash the whole program process.

if we use express, the express server is down.

@heikkihiltunen
Copy link

heikkihiltunen commented Feb 13, 2024

Any news on this? Isn't this very bad...?

@huntharo
Copy link
Contributor

Note that there is a bug in the example program that makes it appear that the error is uncatchable, but the error is, in fact, catchable.

import { finished } from 'stream/promises';

...

sms.end(); // (this does not wait for the writing to finish)

// Wait for writing to finish and throw any caught errors
await finished(sms);

When I add a test for this, it does indeed catch the error thrown from finished.

There is a bug in the library though:

  1. SitemapAndIndexStream is supposed to be managing rotation of files
  2. SitemapAndIndexStream creates a sitemap file before any sitemap items are written - this is sort of a silly case in that it's possible to avoid creating the SitemapAndIndexStream at all in this case - but it's related to the next case which is a real issue
  3. SitemapAndIndexStream closes the current sitemap when the limit is hit AND creates the next sitemap before there is any item to write to it. This then causes the last sitemap to throw if no further items are written. This is bad as this is a perfectly normal case. If the limit is 10k and there are exactly 10k, 20k, 30k, etc. items written there should be 1 index and 1 sitemap and no errors, but instead there is an error

The fix is to split the closing of the current file from the opening of the next file.

@huntharo
Copy link
Contributor

This is related to #362 but this needs specific examples and documentation for SitemapAndIndexStream

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

No branches or pull requests

3 participants