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

tls: remove unnecessary close listener … #34105

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jun 28, 2020

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

ronag commented Jun 28, 2020

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

@ronag ronag 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
Member Author

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

@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)

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 pending requests for changes, 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.

None yet

6 participants