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

Accessing socket after HTTP2 session gets destroyed throws an error #22268

Closed
szmarczak opened this issue Aug 11, 2018 · 5 comments
Closed

Accessing socket after HTTP2 session gets destroyed throws an error #22268

szmarczak opened this issue Aug 11, 2018 · 5 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@szmarczak
Copy link
Member

szmarczak commented Aug 11, 2018

  • Version: v10.8.0
  • Platform: Windows 10 64bit
  • Subsystem: http2

Example:

const http2 = require('http2')
const session = http2.connect(`https://google.com`);
const socket = session.socket;
const req = session.request({':path': '/', ':method': 'GET' });
req.on('close', () => {
	session.close();

	setTimeout(() => {
		socket['example'];
	}, 1000);
});

req.end();
req.resume();
internal/http2/core.js:678
        const value = socket[prop];
                            ^

TypeError: Cannot read property 'example' of undefined
    at Object.get (internal/http2/core.js:678:29)
    at Timeout.setTimeout [as _onTimeout] (/home/szymon/Desktop/bug/example.js:9:9)
    at ontimeout (timers.js:427:11)
    at tryOnTimeout (timers.js:289:5)
    at listOnTimeout (timers.js:252:5)
    at Timer.processTimers (timers.js:212:10)

ProxySocket assumes socket is available, but it's undefined after the session gets destroyed.

If this line was removed:

session[kSocket] = undefined;

it'd work I think.

@szmarczak szmarczak changed the title Accessing socket after HTTP session gets destroyed throws an error Accessing socket after HTTP2 session gets destroyed throws an error Aug 11, 2018
@bzoz
Copy link
Contributor

bzoz commented Aug 13, 2018

Could you provide a complete example?

@szmarczak
Copy link
Member Author

szmarczak commented Aug 14, 2018

Could you provide a complete example?

It is complete. Please note I metioned http2 as the subsystem, so you need const http2 = require('http2'). The options is free to choose. I've updated the example to https://google.com.

@TomCoded
Copy link
Contributor

The example reproduces the error for me with the options below.
(From https://github.com/szmarczak/http2-wrapper/issues/2)

'use strict';
const http2 = require('http2');

const options = {
	'hostname': 'nghttp2.org',
	'protocol': 'https:',
	'path': '/httpbin/get',
	'method': 'GET'
};

const session = http2.connect(`${options.protocol}//${options.hostname}`);
const socket = session.socket;
const req = session.request({':path': options.path, ':method': options.method });
req.on('close', () => {
	session.close();

	setTimeout(() => {
		socket['example'];
	}, 1000);
});

req.end();
req.resume();

It arises from the logic here:

const socket = session[kSocket];
const value = socket[prop];

Assigning to socket['example'] instead of reading from it triggers the same problem a few lines later in core.js

session[kSocket][prop] = value;

@ChALkeR ChALkeR added the http2 Issues or PRs related to the http2 subsystem. label Aug 15, 2018
@jasnell
Copy link
Member

jasnell commented Aug 23, 2018

investigating... working on a fix

@jasnell
Copy link
Member

jasnell commented Aug 23, 2018

#22486 addresses this.

jasnell added a commit to jasnell/node that referenced this issue Aug 24, 2018
addaleax pushed a commit that referenced this issue Aug 27, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Sep 3, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
targos pushed a commit that referenced this issue Sep 6, 2018
Fixes: #22268

PR-URL: #22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 3, 2018
Fixes: nodejs#22268

PR-URL: nodejs#22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
Fixes: nodejs#22268

PR-URL: nodejs#22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
Fixes: #22268

Backport-PR-URL: #22850
PR-URL: #22486
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants