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(encoding/csv/stream): Cancel lineReader if readable is canceled #2401

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

nkronlage
Copy link
Contributor

This fixes an issue with CSVStream where it would leak the file if you canceled the stream before it finished.

The problem was the CSVStream's lineReader was not canceled if the readable stream was canceled.

I've added a test case for this. Note, it only shows up for large files so I've had to introduce a csv file containing many rows of A,B,C.

Before

>  deno test --allow-read encoding/csv/stream_test.ts --filter "leak"
Check file:///Users/neil/github/deno_std/encoding/csv/stream_test.ts
running 1 test from ./encoding/csv/stream_test.ts
[encoding/csv/stream] cancel CSVStream during iteration does not leak file ... FAILED (39ms)

 ERRORS 

[encoding/csv/stream] cancel CSVStream during iteration does not leak file => ./encoding/csv/stream_test.ts:313:6
error: AssertionError: Test case is leaking async ops.

 - 1 async operation to op_read was started in this test, but never completed.

To get more details where ops were leaked, run again with --trace-ops flag.
    at assert (deno:ext/web/00_infra.js:294:13)
    at asyncOpSanitizer (deno:runtime/js/40_testing.js:228:13)
    at async resourceSanitizer (deno:runtime/js/40_testing.js:375:7)
    at async exitSanitizer (deno:runtime/js/40_testing.js:432:9)
    at async Object.applyPermissions [as fn] (deno:runtime/js/40_testing.js:533:9)
    at async runTest (deno:runtime/js/40_testing.js:813:7)
    at async Object.runTests (deno:runtime/js/40_testing.js:1095:22)

 FAILURES 

[encoding/csv/stream] cancel CSVStream during iteration does not leak file => ./encoding/csv/stream_test.ts:313:6

FAILED | 0 passed | 1 failed | 3 filtered out (78ms)

error: Test failed

After

> deno test --allow-read encoding/csv/stream_test.ts --filter "leak"
Check file:///Users/neil/github/deno_std/encoding/csv/stream_test.ts
running 1 test from ./encoding/csv/stream_test.ts
[encoding/csv/stream] cancel CSVStream during iteration does not leak file ... ok (34ms)

ok | 1 passed | 0 failed | 3 filtered out (67ms)

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@kt3k kt3k left a comment

Choose a reason for hiding this comment

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

@nkronlage Sorry for the delay in review. Good catch! Thanks!

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

3 participants