fix a handful of streaming parser bugs #1570

yocontra · 2020-12-17T20:06:20Z

Summary

We use the streaming parser and encountered a number of issues - sometimes files would stall out, sometimes they would always fail unless you opened and resaved the file, sometimes dates would just not be parsed for some files.

Major Changes

Change default arguments to match the docs (Closes docs: fix default options for streaming xlsx reader #1531)
Fixes a race condition where worksheets would start being read before the styles were parsed, causing dates to fail and show up as numbers (Closes [BUG] date parsing doesn't work in streaming mode #1430)
Fixes a stall (unreported?) in the BaseXForm parser - if closetag was encountered it would return, and the sax parser would fail to flush and just pause forever. The correct behavior should be to break so that it flushes the parser correctly.
Fixes a stall (unreported?) where worksheets would never be read.
- I resolved this by implementing a basic queue to handle the waitingWorksheets logic - I think this cleans up the code a bit as well and makes it easier to understand.
Removes the iterateStream utility, which was causing stalls and race conditions (Closes [bug] iterate-stream causes race conditions in streaming mode #1558)
- I read through all of the previous conversation on the unzipper project so I understand the initial reason why it was added.
- All tests pass without it, I think this was just covering up for some of the previous bugs - file parser generators were stalling which bubbles up causing the unzipper generator to stall as well (and thus fail to autodrain, because that statement was never reached).

Minor Changes

Changes the getStream ducktype to match the other code in the project (just checking for a pipe function)
Use pipeline when writing files so that errors are properly managed
Removed some node 8 code - Node 8 has been deprecated/EOL since 2019 (https://blog.risingstack.com/update-nodejs-8-end-of-life-no-support/)
Switches to the promise version of autodrain, since your PR to unzipper making that work correctly landed and is published

Test plan

All of our edge case tests that use exceljs under the hood pass, and the existing exceljs tests pass. Let me know if you would like to see any additional tests.

Siemienik

Hi @contra, Thank you for the great contribution :) Great job 🥇

Could I please you to do two points?

Why test on MacOS with node v10 & v12 fails?
Update this branch to the current master.

As these changes are major (min node version), I suppose we should separate major changes from another. To protect backwards-compatibility.

Additionally, I wan't to merge it without other contribution also approve major changes.

I'm contributing ExcelJS just for fun. If want you to know something more go into #1500

Siemienik · 2021-02-15T22:47:54Z

lib/stream/xlsx/workbook-reader.js

-      entries: 'ignore',
+      hyperlinks: 'cache',
+      styles: 'cache',
+      entries: 'emit',


I suppose that you have important reason here, so could I please you to explain, why changing these settings is obviusly? I'm little worried about losing performance here.

Imo, change defaults parameters should fails tests, what doesn't happen, hmm...

@Siemienik I was asked to do this by @alubbe #1531 - the docs do not currently match the implementation, so this makes the implementation match the docs which is the intended behavior.

lib/stream/xlsx/workbook-reader.js

Siemienik · 2021-02-15T22:55:48Z

lib/stream/xlsx/workbook-reader.js

+  async *flushQueue(force) {
+    if (!force && !this.isDirectReady()) return; // not ready yet! do nothing
+    if (this.waitingWorkSheets.length === 0) return; // queue flushed!
+    const queue = this.waitingWorkSheets; // make a copy


May I propose to pass waitingWorkSheets as an argument? It should be much safer.

@Siemienik flushQueue can be called multiple times asynchronously so there needs to be some mutable shared state with the worksheet queue on it - the alternative here would be passing an object in with the queue from the parent function. Based on everything else in this library it looked like mutable state living on the class was the preferred pattern.

lib/stream/xlsx/workbook-reader.js

Siemienik · 2021-02-15T23:06:30Z

lib/xlsx/xform/base-xform.js

-          if (!this.parseClose(value.name)) {
-            return this.model;
-          }
+          if (!this.parseClose(value.name)) break;


which issue is it fix?

"Fixes a stall (unreported?) in the BaseXForm parser - if closetag was encountered it would return, and the sax parser would fail to flush and just pause forever. The correct behavior should be to break so that it flushes the parser correctly."

Siemienik · 2021-02-15T23:06:36Z

lib/xlsx/xlsx.js

        let stream;
-        if (entryName.match(/xl\/media\//) ||
+        if (
+          entryName.match(/xl\/media\//) ||


which issue is it fix?

I believe this was just from running the project linter, if not I can revert this

yocontra · 2021-02-17T05:32:55Z

@Siemienik Most of these changes are not going to be easy to land in a node 8 compatible way so I would suggest releasing this as a major or a part of the next major if one is planned.

If anyone feels strongly that this needs to be done in a node 8 compatible way feel free to amend this PR but I don't have the capacity to take that on, this was already a significant amount of debugging and work to sort this out.

yocontra · 2021-02-17T05:46:12Z

@Siemienik Rebased and addressed the PR feedback. For the Mac issue - not sure, would have to look into how github runs their mac tests. It is pretty weird for it to fail only on mac when this entire suite is pretty OS independent.

jcalderonzumba · 2022-11-10T10:08:03Z

@yocontra @Siemienik is there any progress on this one?
We are happy users of your library and this PR will help us close an outstanding issue we have with some excel files that we process in our system.

Is there anything I can do on my side to get this moved and merged?

Siemienik · 2022-11-10T14:18:07Z

I would like too. However I am overloaded with my regular job.

yocontra · 2022-11-16T17:58:23Z

@jcalderonzumba Could you test my fork against your workload and see if it resolves your issues? That would be helpful.

It looks like https://github.com/coursebase/exceljs is also a fork that incorporates my changes in this PR and some other fixes (unzipper) that were causing streaming issues.

lucalas · 2023-10-03T09:33:20Z

@Siemienik do you think there is a chance to review this pr and merge it?
We tried this fix and worked for us.

albanm · 2024-02-13T13:12:02Z

I fully understand that sometimes time is lacking to maintain a library and that everybody here is working for free. But honestly these fixes are pretty important, IMO either these are fixed or the library should be considered as abandoned. At least there should be a disclaimer in the doc about the streaming parser having known issues, and the default options should be documented properly (styles being 'ignore' while documented as 'cache' is pretty bad).

Is there any hope of seeing this merged anytime ? Or does someone know a maintained fork that includes these fixes ?

yocontra · 2024-02-13T18:56:18Z

@albanm You can always stick "github:yocontra/exceljs#fix-streams" in your package.json as the version to use this PR's version.

hcminh · 2024-02-22T10:48:09Z

is there any update on this, I really need this fixed parser 😢

enjogole · 2024-05-06T23:14:59Z

@yocontra I tried using your fork, but it still has a problem unless you opened and resaved the file, was this resolved?

Thanks.

mo-gad · 2024-06-11T11:19:42Z

@Siemienik: This fix solves the issue we face as well (verified with "github:yocontra/exceljs#fix-streams"). Would really appreciate a final review and merge. Thanks a lot!

moonjoungyoung · 2024-10-24T01:55:13Z

Hello. Any changes on this PR?

yocontra · 2025-01-27T02:36:28Z

Would one of the maintainers want to take over this PR and get it across the finish line? It seems like it needs some very minor love. Due to a new role I'm not able to allocate any time to this - but would like to see it land eventually since we've been going back and forth on it (and me rebasing it) for a little over 4 years.

Totally understand if maintainers are busy and maybe somebody else wants to fork off my branch and resubmit instead.

matteovivona · 2025-01-27T08:29:52Z

@yocontra I used pnpm patch to create a local patch with your PR

Siemienik self-requested a review February 12, 2021 02:29

yocontra mentioned this pull request Feb 15, 2021

TypeError: Cannot read property 'sheets' of undefined #962

Open

Siemienik reviewed Feb 15, 2021

View reviewed changes

Siemienik requested review from Alanscut, alubbe and guyonroche February 15, 2021 23:09

yocontra added 10 commits February 16, 2021 22:43

fix: first pass at fixing stream bugs

941ebb2

fix: finish streaming improvements

fa54a44

fix: zip stream error mgmt

02a8497

fix: race condition with parsing styles

884aa57

fix: tests

28f5223

fix: resolve queue not being flushed sometimes, clean up queue code

376c697

fix: clean up queue, change defaults

c276f6e

fix: remove wrapper stream

4a51eb4

chore: remove remaining node 8 code

5393378

chore: PR feedback

4dcf2de

yocontra force-pushed the fix-streams branch from 24d322d to 4dcf2de Compare February 17, 2021 05:45

chore: add node 15 to matrix

25e38de

Siemienik self-requested a review February 20, 2021 08:12

Siemienik and others added 2 commits November 11, 2021 13:47

Merge branch 'master' into fix-streams

7a9c788

Merge branch 'master' into fix-streams

50f56f1

CarlOlson added a commit to coursebase/exceljs that referenced this pull request Nov 2, 2022

add exceljs/exceljs#1570 changes

5908d77

silto mentioned this pull request Apr 4, 2025

Divers fix registre MTES-MCT/trackdechets#4091

Merged

4 tasks

fix a handful of streaming parser bugs #1570

Are you sure you want to change the base?

fix a handful of streaming parser bugs #1570

Uh oh!

Conversation

yocontra commented Dec 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Major Changes

Minor Changes

Test plan

Uh oh!

Siemienik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yocontra commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yocontra commented Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcalderonzumba commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Siemienik commented Nov 10, 2022

Uh oh!

yocontra commented Nov 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lucalas commented Oct 3, 2023

Uh oh!

albanm commented Feb 13, 2024

Uh oh!

yocontra commented Feb 13, 2024

Uh oh!

hcminh commented Feb 22, 2024

Uh oh!

enjogole commented May 6, 2024

Uh oh!

mo-gad commented Jun 11, 2024

Uh oh!

moonjoungyoung commented Oct 24, 2024

Uh oh!

yocontra commented Jan 27, 2025

Uh oh!

matteovivona commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

yocontra commented Dec 17, 2020 •

edited

Loading

yocontra commented Feb 17, 2021 •

edited

Loading

yocontra commented Feb 17, 2021 •

edited

Loading

jcalderonzumba commented Nov 10, 2022 •

edited

Loading

yocontra commented Nov 16, 2022 •

edited

Loading