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

http.ServerResponse is not an instance of stream.Writable? #44188

Closed
sosoba opened this issue Aug 9, 2022 · 9 comments · Fixed by #45642 or restatedev/sdk-typescript#426 · May be fixed by #45834
Closed

http.ServerResponse is not an instance of stream.Writable? #44188

sosoba opened this issue Aug 9, 2022 · 9 comments · Fixed by #45642 or restatedev/sdk-typescript#426 · May be fixed by #45834
Labels
confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. web streams

Comments

@sosoba
Copy link
Contributor

sosoba commented Aug 9, 2022

Version

18.7.0

Platform

Microsoft Windows NT 10.0.19044.0 x64

Subsystem

stream

What steps will reproduce the bug?

import { createServer } from 'node:http';
import { Writable } from 'node:stream';
import { once } from 'node:events';

const server = createServer(async (req, nodeStreamResponse) => {
  const webStreamResponse = Writable.toWeb(nodeStreamResponse);
});
server.listen({ port: 8080 });
await once(server, 'listening');
await fetch('http://localhost:8080');

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

According to the documentation:

Class: http.ServerResponse extends http.OutgoingMessage
Class: http.OutgoingMessage extends Stream

This means that ServerResponse fulfilling the contract Writable.toWeb.

What do you see instead?

TypeError [ERR_INVALID_ARG_TYPE]: The "streamWritable" argument must be an stream.Writable. Received an instance of ServerResponse
    at new NodeError (node:internal/errors:387:5)
    at Object.newWritableStreamFromStreamWritable (node:internal/webstreams/adapters:99:11)
    at Writable.toWeb (node:internal/streams/writable:926:27)  

Additional information

Maybe this condition:

if (typeof streamWritable?._writableState !== 'object') {

is too heavy?

@MoLow MoLow added the http Issues or PRs related to the http subsystem. label Aug 9, 2022
@daeyeon
Copy link
Member

daeyeon commented Aug 14, 2022

http.ServerResponse is not an instance of stream.Writable?

Throwing an error here seems to be an expected behavior since http.ServerResponse is an instance of Stream, not stream.Writable. Is this issue a feature request for the following?

stream.Writable.toWeb(stream : <stream.Writable>|<http.OutgoingMessage>)

@daeyeon daeyeon added stream Issues and PRs related to the stream subsystem. web streams labels Aug 14, 2022
@sosoba
Copy link
Contributor Author

sosoba commented Aug 17, 2022

It seems that the bug is on @types/node

class OutgoingMessage extends stream.Writable {

Of course, the @daeyeon proposition is successful.

stream.Writable.toWeb(stream : <stream.Writable>|<http.OutgoingMessage>)

@ronag ronag added the confirmed-bug Issues with confirmed bugs. label Nov 4, 2022
@ronag
Copy link
Member

ronag commented Nov 4, 2022

toWeb should work on OutgoingMessage.

@ronag ronag added the good first issue Issues that are suitable for first-time contributors. label Nov 4, 2022
@daltonna
Copy link

daltonna commented Nov 9, 2022

Has anyone claimed this issue? I haven't contributed before but I think this would be a good first issue for me as I have some ideas on a fix.

@AlexKliger
Copy link

Was this issue already assigned? If not, I'd like to try and solve it. Thanks.

@cola119
Copy link
Member

cola119 commented Nov 20, 2022

go for it :)

debadree25 added a commit to debadree25/node that referenced this issue Nov 27, 2022
Attempted to fix the issue by watering down the condition being checked in lib/internal/webstreams/adapters.js similar to
isWritableNodeStream utility being used in internal/streams/utils

Fixes: nodejs#44188
debadree25 added a commit to debadree25/node that referenced this issue Nov 28, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: nodejs#44188
@zeazad-hub
Copy link

Hey, is this issue still open, because I would like to work on it as my first issue.

@zeazad-hub
Copy link

Hello, I found out what was going wrong. Our outgoing message defined here does not extend stream.writable. The probelm is that stream.writable has a writable attribute that behaves differently from the OutgoingMessage writabel attribute and this causes test/parallel/test-http-writable-true-after-close.js to fail because it is not writable after it is destroyed. I was wondering if OutgoingMessage.writable should be true when OutgoingMessage.destroyed is true (like the test checks for) or should OutgoingMessage.writable be false when it gets destroyed?

zeazad-hub added a commit to zeazad-hub/node that referenced this issue Dec 8, 2022
… instead of stream

http.OutgoingMessage is supposed to be an instance of stream.Writable, so I set
the prototype of OutgoingMessage equal to the Writable prototype and I called
the Writable consrtuctor at the end of the constructor for OutgoingMessage. I
made sure to override any methods and prpoperties in Writable that were
already defined in OutgoingMessage in order to not interfere with the
current behavior of OutgoingMessage.

Fixes: nodejs#44188
@daeyeon
Copy link
Member

daeyeon commented Dec 9, 2022

@zeazad-hub FWIW, there seems to be a performance issue when trying OutgoingMessage inheriting stream.Writable.

nodejs-github-bot pushed a commit that referenced this issue Dec 10, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
ErickWendel pushed a commit to ErickWendel/node that referenced this issue Dec 12, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: nodejs#44188
PR-URL: nodejs#45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
targos pushed a commit that referenced this issue Dec 12, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
zeazad-hub added a commit to zeazad-hub/node that referenced this issue Dec 12, 2022
… instead of stream

http.OutgoingMessage is supposed to be an instance of stream.Writable, so I set
the prototype of OutgoingMessage equal to the Writable prototype and I called
the Writable consrtuctor at the end of the constructor for OutgoingMessage. I
made sure to override any methods and prpoperties in Writable that were
already defined in OutgoingMessage in order to not interfere with the
current behavior of OutgoingMessage.

Fixes: nodejs#44188
zeazad-hub added a commit to zeazad-hub/node that referenced this issue Dec 12, 2022
I accidentally left a script.sh file that copied all of the
test files into a different directory in a previous commit.
I did not end up using the script.sh and have now removed it
from the repo.

Fixes: nodejs#44188
targos pushed a commit that referenced this issue Dec 13, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this issue Dec 30, 2022
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this issue Jan 3, 2023
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this issue Jan 4, 2023
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
danielleadams pushed a commit that referenced this issue Jan 5, 2023
Attempted to fix the issue by watering down the condition being
checked in internal/streams/utils isWritableNodeStream utility

Fixes: #44188
PR-URL: #45642
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem. stream Issues and PRs related to the stream subsystem. web streams
Projects
None yet
8 participants