Skip to content

tls: remove unnecessary close listener … - #34105

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:tls-close-listener
Closed

tls: remove unnecessary close listener …#34105
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:tls-close-listener

Conversation

@ronag

Copy link
Copy Markdown
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
@ronag
ronag requested a review from addaleaxJune 28, 2020 18:35
@ronag

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@addaleax

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@ronag

ronag commented Jul 1, 2020

Copy link
Copy Markdown
MemberAuthor

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 <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@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 <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
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 <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
@codebyterecodebytere mentioned this pull request Sep 28, 2020
@panmenghan

Copy link
Copy Markdown

@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