Skip to content

Commit

Permalink
Streams: return buffers from ReadStream#readBuffer, not strings
Browse files Browse the repository at this point in the history
  • Loading branch information
@Kaiepi
Kaiepi committed Aug 11, 2020
1 parent 5cee3b4 commit e91c4c5
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion lib/streams.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ export class ReadStream {

async readBuffer(byteCount: number | null = null) {
await this.loadIntoBuffer(byteCount, true);
const out = this.peek(byteCount) as Buffer | null;
const out = this.peekBuffer(byteCount);
if (byteCount === null || byteCount >= this.bufSize) {
this.bufStart = 0;
this.bufEnd = 0;
Expand Down

7 comments on commit e91c4c5

@Zarel
Copy link
Member

@Zarel Zarel commented on e91c4c5 Aug 11, 2020

Choose a reason for hiding this comment

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

efa0af0#diff-4addb8467b884244c38b8e9762b165ceR255

Wow, apparently this was my fault. I wonder how that happened.

@urkerab
Copy link
Contributor

Choose a reason for hiding this comment

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

What I don't understand is why there isn't an await here (or in read()).

@Zarel
Copy link
Member

@Zarel Zarel commented on e91c4c5 Aug 11, 2020

Choose a reason for hiding this comment

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

That's a good point, there probably should be.

@Zarel
Copy link
Member

@Zarel Zarel commented on e91c4c5 Nov 1, 2020

Choose a reason for hiding this comment

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

@urkerab it turns out no, there 100% should not have been an await here:

8b68cdd

@urkerab
Copy link
Contributor

@urkerab urkerab commented on e91c4c5 Nov 1, 2020

Choose a reason for hiding this comment

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

Fair enough, but in that case I think you should have either reverted it to the version here without the cast, or more readably, assert that the type is a Promise<>, as that makes it clearer that you're not waiting for it.

@Zarel
Copy link
Member

@Zarel Zarel commented on e91c4c5 Nov 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@urkerab the assert is intentional; this.peekBuffer returns Buffer | null | Promise<Buffer | null>, and if you call loadIntoBuffer immediately before it, it is guaranteed not to return a Promise.

...except in the case of a race condition from the await for the loadIntoBuffer... if you call read before the previous read has finished. Which is unsupported but perhaps should not cause spectacular bugs. Hm.

@Zarel
Copy link
Member

@Zarel Zarel commented on e91c4c5 Nov 2, 2020

Choose a reason for hiding this comment

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

I pushed c8dfed1 which no longer casts and protects against the race.

We can support multiple reads in a row, but we have too many other things to support, so I'm putting that off.

Please sign in to comment.