Skip to content

fix: support responses exceeding high watermark#782

Merged
kettanaito merged 9 commits into
feat/net-v2from
Michael/fix-big-response
May 11, 2026
Merged

fix: support responses exceeding high watermark#782
kettanaito merged 9 commits into
feat/net-v2from
Michael/fix-big-response

Conversation

@mikicho
Copy link
Copy Markdown
Member

@mikicho mikicho commented May 7, 2026

  1. Fix interceptor reapply
  2. Forward drain event to the server response socket
  3. Fixed socket.address family string case

@mikicho mikicho requested a review from kettanaito May 7, 2026 21:35
await expect(response.text()).resolves.toBe('original-response')
})

it('big response', async () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you tell us more what was the problem big responses were exhibiting? Was there some error being thrown? Let's give this test case a descriptive name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

At the consumer level, it checks whether we support 1MB responses. On a technical level, it verifies that we properly forward drain events for streams larger than the high water mark.

})

it('big response', async () => {
const responseBody = new Array(1024 * 1024 + 1).join('.')
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to create a Uint8Array of the same size instead?

Comment thread src/interceptor.ts
protected abstract setup(): void

public apply(): void {
if (this.readyState === InterceptorReadyState.DISPOSED) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But we still need a guard to prevent calling apply() when the interceptor is already applied. I believe the fix here is to change:

if (this.readyState === InterceptorReadyState.ACTIVE) {

Comment thread src/Interceptor.test.ts Outdated
expect(listener).not.toHaveBeenCalled()
})

it('can apply after disposal', () => {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
it('can apply after disposal', () => {
it('applies the interceptor after disposal', () => {

Comment thread src/Interceptor.test.ts Outdated
})

it('can apply after disposal', () => {
class MyInterceptor extends Interceptor<{ test: TypedEvent }> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
class MyInterceptor extends Interceptor<{ test: TypedEvent }> {
class MyInterceptor extends Interceptor<{}> {

Or any, if it accepts it.

Comment thread src/Interceptor.test.ts Outdated
}
const interceptor = new MyInterceptor()

interceptor.apply()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we also need readyState assertions after individual actions? That way, we can guard against false positive/negatives by making sure every state transitions indeed produces the correct state (readyState).

@mikicho mikicho requested a review from kettanaito May 8, 2026 20:06
@kettanaito kettanaito changed the title fix: big response and reapply fix: support responses exceeding high watermark May 11, 2026
Copy link
Copy Markdown
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

Looks good to me!

I polished a few things, fixed the interceptors.test.ts file name casing.

@kettanaito kettanaito merged commit 0b1ede0 into feat/net-v2 May 11, 2026
5 of 7 checks passed
@kettanaito kettanaito deleted the Michael/fix-big-response branch May 11, 2026 16:49
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.

2 participants