Skip to content

Conversation

@ronag
Copy link
Member

Wrapped streams are expected to behave the same as socket with handle.
Remove unnecessary difference in handling.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Wrapped streams are expected to behave the same as socket with handle. Remove unnecessary difference in handling.
@ronagronag added the tls Issues and PRs related to the tls subsystem. label Jun 28, 2020
@ronagronag requested a review from addaleaxJune 28, 2020 18:35
@ronag
Copy link
MemberAuthor

As a side note... there is a lot that could be improved in the tls code... However, it's kind of risky and difficult to work with. Is this something we would like to do or should I just leave it alone?

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member

As a side note... there is a lot that could be improved in the tls code... However, it's kind of risky and difficult to work with. Is this something we would like to do or should I just leave it alone?

I think it’s similar to streams – it’s hard to work with until somebody puts in a lot of work to clear it up. :/

@ronagronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 29, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
MemberAuthor

ronag commented Jul 1, 2020

Landed in 60a217b

MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
Wrapped streams are expected to behave the same as socket with handle. Remove unnecessary difference in handling. PR-URL: #34105 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Wrapped streams are expected to behave the same as socket with handle. Remove unnecessary difference in handling. PR-URL: #34105 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Wrapped streams are expected to behave the same as socket with handle. Remove unnecessary difference in handling. PR-URL: #34105 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
@panmenghan
Copy link

@ronag
This change causes the following code to crash.

consthttp2=require('http2')constnet=require('net')const{Duplex}=require('stream')classJsSocketextendsDuplex{constructor(socket){super({autoDestroy: true,allowHalfOpen: false})socket.on('data',(data)=>this.push(data))socket.on('end',(data)=>this.push(null))socket.on('close',()=>this.destroy())this.socket=socket}unref(){}ref(){}setNoDelay(){}setTimeout(){}_write(data,encoding,callback){this.socket.write(data,encoding,callback)}_final(callback){callback()}_read(size){}_destroy(error,callback){callback()}}constrealSocket=net.connect({host: 'www.example.com',port: '443'},()=>{console.log('connected')constconnection=http2.connect('https://www.example.com',{socket: newJsSocket(realSocket),// crash!!!// socket: realSocket, // but not crash!!!})consth2Stream=connection.request({[http2.constants.HTTP2_HEADER_PATH]: '/'})setTimeout(()=>{realSocket.destroy()},2e3)setTimeout(()=>{connection.close()},3e3)})

output:

internal/js_stream_socket.js:210 handle.finishWrite(req, errCode); ^ TypeError: Cannot read property 'finishWrite' of null at JSStreamSocket.finishWrite (internal/js_stream_socket.js:210:12) at Immediate.<anonymous> (internal/js_stream_socket.js:195:14) at processImmediate (internal/timers.js:461:21) 

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.tlsIssues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@ronag@nodejs-github-bot@addaleax@panmenghan@trivikr@rickyes