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

stream.unshift() after end event #330

Closed
HitkoDev opened this issue Feb 11, 2020 · 12 comments · Fixed by #334
Closed

stream.unshift() after end event #330

HitkoDev opened this issue Feb 11, 2020 · 12 comments · Fixed by #334
Assignees
Labels

Comments

@HitkoDev
Copy link

In some cases calling readableStream.read can reach the end of the stream (e.g. on a short file, or when stream emits everything in a single chunk), or stream may end for other reasons, causing readableStream.unshift to throw an error.

@Borewit
Copy link
Collaborator

Borewit commented Feb 17, 2020

@HitkoDev Can you provide a file and description how to reproduce this issue?

@HitkoDev
Copy link
Author

HitkoDev commented Feb 17, 2020

Sure:

const buff = Buffer.from(whatever);

class MyStream extends Readable {
  _read() {
    this.push(buff);
    this.push(null); 
  }
}

const s = new MyStream();
fileType.stream(s).then(res => console.log(res.fileType));

@bencmbrook
Copy link
Contributor

Interesting.. Our test fixtures should definitely cover this case. Did anything change with the .stream tests?

@bencmbrook
Copy link
Contributor

@HitkoDev are you reading from your stream before calling fileType.stream?

@HitkoDev
Copy link
Author

HitkoDev commented Feb 17, 2020

@bencmbrook I'm not reading from source stream, I'm always reading from the stream returned by fileType. As I said, this usually happens when dealing with really small files, file uploads, or any other readable stream where everything is returned in a single chunk and there's nothing left after calling

file-type/core.js

Line 1287 in 99beda8

const chunk = readableStream.read(minimumBytes) || readableStream.read() || Buffer.alloc(0);

@bencmbrook
Copy link
Contributor

Fixed it on my branch, thanks.

Hey @sindresorhus, any chance I can get repo permissions to PR without a fork?

@sindresorhus
Copy link
Owner

@bencmbrook Oh yeah. I must have forgotten to do that previously... Done. Prefer PRs over pushing directly to master though.

@Borewit Borewit assigned Borewit and bencmbrook and unassigned Borewit Feb 20, 2020
@Borewit
Copy link
Collaborator

Borewit commented Feb 20, 2020

Are you taking care of this one @bencmbrook?

@bencmbrook
Copy link
Contributor

bencmbrook commented Feb 20, 2020

@Borewit yes I have a fix, but @sindresorhus I don't think I received the access rights?

git push --set-upstream  origin bencmbrook/short-stream
ERROR: Permission to sindresorhus/file-type.git denied to bencmbrook.
fatal: Could not read from remote repository.

In the interest of time, my change to core.js is

file-type/core.js

Line 1295 in 99beda8

readableStream.unshift(chunk);

to:

		if (readableStream.readableEnded || readableStream._readableState.ended) {
			readableStream.unshift();
		} else {
			readableStream.unshift(chunk);
		}

and a test for test.js

test('.stream() method - short stream', async t => {
	const buffer = Buffer.from([0, 1, 0, 1]);
	class MyStream extends stream.Readable {
		_read() {
			this.push(buffer);
			this.push(null);
		}
	}

	const shortStream = new MyStream();
	const newStream = await FileType.stream(shortStream);
	t.is(newStream.fileType, undefined);
});

@Borewit
Copy link
Collaborator

Borewit commented Feb 20, 2020

Hmm, via comments, is not really the way I guess. but it is good news you took care of the issue.

Maybe push it via your cloned file-type repo?

@sindresorhus
Copy link
Owner

@bencmbrook You have to accept the invitation.

@bencmbrook
Copy link
Contributor

Hah, my bad. Received, thank you.

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

Successfully merging a pull request may close this issue.

4 participants