FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

tls: remove unnecessary close listener … by ronag · Pull Request #34105 · nodejs/node · GitHub

/ node Public

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 commented Jun 28, 2020

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.
ronag added the tls Issues and PRs related to the tls subsystem. label Jun 28, 2020
ronag requested a review from addaleax June 28, 2020 18:35

ronag commented Jun 28, 2020

Copy link
Copy Markdown
Member Author

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?

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

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. :/

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jun 29, 2020

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

ronag commented Jul 1, 2020

Copy link
Copy Markdown
Member Author

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>
MylesBorins 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>
codebytere mentioned this pull request Sep 28, 2020

Copy link
Copy Markdown

@ronag
This change causes the following code to crash.

const http2 = require('http2')
const net = require('net')
const {Duplex} = require('stream')

class JsSocket extends Duplex {
  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()
  }
}

const realSocket = net.connect({host: 'www.example.com', port: '443'},
  () => {
    console.log('connected')
    const connection = http2.connect('https://www.example.com', {
      socket: new JsSocket(realSocket), // crash!!!
      // socket: realSocket, // but not crash!!!
    })
    const h2Stream = 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)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. tls Issues and PRs related to the tls subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants


Back | FazBrowse Home | New Git URL