Skip to content

Realease ReadableStream reader lock#12

Closed
Gozala wants to merge 15 commits intoachingbrain:masterfrom
Gozala:releas-lock
Closed

Realease ReadableStream reader lock#12
Gozala wants to merge 15 commits intoachingbrain:masterfrom
Gozala:releas-lock

Conversation

@Gozala
Copy link
Contributor

@Gozala Gozala commented Aug 7, 2020

This fixes #8 and is based on #9. Actual changes are in last commit.

Copy link
Owner

@achingbrain achingbrain left a comment

Choose a reason for hiding this comment

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

LGTM, just a few inline comments.


server = http.createServer((req, res) => {
if (req.method === 'POST' &&
// @ts-ignore - header may not be present
Copy link
Owner

Choose a reason for hiding this comment

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

If it's not present this will error so it should null-guard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not bother because it's a test and all request will have it.

@achingbrain
Copy link
Owner

It would have been better to split the release-lock PR out from the types PR, the two things are not related.

@Gozala
Copy link
Contributor Author

Gozala commented Aug 17, 2020

It would have been better to split the release-lock PR out from the types PR, the two things are not related.

They were independent already, as per PR description:

This fixes #8 and is based on #9. Actual changes are in last commit.

I was hoping that #9 would land first, in which case this would contain no type related changes.

I did however added few commits to address some of the feedback that was not related to release lock, but I'll cherry-pick them into #9 as well.

@Gozala Gozala requested a review from achingbrain August 17, 2020 23:25
@achingbrain
Copy link
Owner

Fixed in 4480ea1

@achingbrain achingbrain closed this Oct 7, 2020
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.

browser-readablestream-to-it should release reader lock and cancel stream on break

2 participants