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.

Sign up for GitHub

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

Jump to bottom

difference between http2 Compatibility API and http #20060

Closed
xxoo opened this issue Apr 16, 2018 · 18 comments
Closed

difference between http2 Compatibility API and http #20060

xxoo opened this issue Apr 16, 2018 · 18 comments
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.

Comments

@xxoo
Copy link

xxoo commented Apr 16, 2018 •
edited
Loading

  • Version: 9.11.1
  • Platform: macos
  • Subsystem:
let http2 = require('http2');
let http = require('http');
http2.createSecureServer({
	allowHTTP1: true,
	cert: `-----BEGIN CERTIFICATE-----
MIICpDCCAYwCCQDchnaEFAYYfzANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls
b2NhbGhvc3QwHhcNMTcxMTExMjA1NjQ2WhcNMTcxMjExMjA1NjQ2WjAUMRIwEAYD
VQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDP
CdzfoRXMW4rHYSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAq
mJfsiN1GRfMmO+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb
+vAYECKwWB9WJvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFy
dTnE2OOSx98ZG/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs
2dDrclBdw1gjIFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiL
uj3B31JYCCpr/fFWpuJ9AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAExF+pAYc0Kq
ejSiNxu4Ae5T5XLXOCFSGHRjHwE/Wpud0SqJIrLAAm6VfyU/MDvEIUlwfnHaHpyL
/j+1JNCLlMPcCY81k3CM3FrgnYit9ImU9ni3DU+c23zdMrkiF1bgQZBdbg2gzqgy
8yq2Ws72a6i0S8odCQ4/inCall3D9c64sefE9LocLTEBrTQDYp+bg6+RX7QKdmE6
6dkYm4oQOeB1lb0IdV+YSKz/bkypFPA0KY5dtpsoPK8TpwwUzg9cbnvXo0gKMbR3
qY8JmgsajfMbbynqR88kFHbtHOISC/XtmlObxSA+CqCKrI+f9ljc+wADPkF+rqxX
Gs1T2qkuJ6g=
-----END CERTIFICATE-----`,
	key: `-----BEGIN PRIVATE KEY-----
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDPCdzfoRXMW4rH
YSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAqmJfsiN1GRfMm
O+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb+vAYECKwWB9W
JvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFydTnE2OOSx98Z
G/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs2dDrclBdw1gj
IFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiLuj3B31JYCCpr
/fFWpuJ9AgMBAAECggEABZxO0ACdhpw0dpK7ZE3uiXEU3McFH4jq332JUvmbrLNN
PCns1w8EbCLsIhMCr9Ogu4bHFzHeTTk4DaEyECZK2ky5+5u8pVBlDaODF2unvWIn
YhmNHrIS5bGA28PB4ErYl12FhCFrzzuUHdGQqR1Vicb5U7I3MpvnOq6BKJGbwN3J
ZyEhlZ9MirbAQa12yv5h3827RkLeFXqzmxMHzKgT8VXbQwenZ3HzhwZ5jNd65g6M
yhBQw9tJvwjapm/gj66DpVdV9gJhzi5TBdRsIe5yu+QVInN0FKPCH7mpIfy3I+j4
0uYIXr/DLjv6sok6zQqvICQrhRwCQ557qsmGc5V8AQKBgQD/H/+iKzpqNgHYrgmF
ROPlb8bAVFslqKjKWiH7q6wUV2htQ6+S9giWxbmUXb5j2qG26YaEtPIcm+g5LEXm
Ox9MQjhNEeQIhb6KH5ghHrIDxqQNT2+sR49VbR3Y+OL6oBxFOv00nwg/h8KBK9NN
/asR15NmCka36OMwVinlr1xujQKBgQDPv6TcBuXF5b77h3OYoURnXv2ZJvULIDTV
pKaJa9Z/k8bCiTiHbKNjVGsfDijXEd82Dp6YwJMbYIAlexewR48vpQMOnGOkA9Hk
emMBfMy0t4i+8tZTT0UdYgVICsbZQSO/8L0LcEkGVfmRgJPrUhDw9o9AeD2YLnXM
7d9mBCD/sQKBgQDxhf6BLQFpKWXIFsLGmrhRLedvjqyXUzswDfIcCqKmwzUGM8zU
iP0Kl3cfwTuL1p+/xQZnPdHzSZmn/oTR9+iiThJ0y9ogQ1Vl95ES0bdfIb+PJkOn
SjukeN+H198xuz/oPncVSPULB+AYXz/0lpBMHNTbBiF63AuwZ/HUEpajxQKBgH8f
z1rQYbQaZSZ3eVXxgPEcYGRiQVpgh9Qf38SBl40TuXF7FHtSEB0NIEutl3IbvpHO
ml/wn1QGVgQZcaJt94F5IQjEy/gmWj7MYV8cpgsDsArggCQUgr97Jq4x4gI5aQ3f
215vhE/7Ni9CFcHOww0gYwJZUZ+Y9n7DJIvBhQvRAoGBAP68uVfOB7oVV3z9K6G+
oJHntre6Za8deemnksM/5H4/k4Kedl+RIMExhLsAypaP0aIZPW29Mwv3eT19ciUm
g6KrdhUNWvbNhryyg//VYnDLQi0FnGr+oZDQNhGxNdWXjxOBBTVxe4Z16CEDgIXI
Pwz9a9ZgvBd2si5/cMwM6aOe
-----END PRIVATE KEY-----`
}, onmessage).listen(1234);
http.createServer(onmessage).listen(5678);

function onmessage(req, res){
	if (req.method === 'GET') {
		res.writeHead(200, {
			'content-type': 'text/html'
		});
		res.end(`<!DOCTYPE html>
<html>
	<head>
		<meta charset="utf-8"/>
		<meta http-equiv="X-UA-Compatible" content="IE=Edge"/>
		<meta name="format-detection" content="telephone=no"/>
		<title>fusion-backend</title>
	</head>
	<body>welcome
		<input type="file"/>
		<script>
			document.querySelector('input').addEventListener('change', function() {
				if (this.files.length) {
					let x = new XMLHttpRequest();
					x.open('put', '/images/' + this.files[0].name, true);
					x.onreadystatechange = function() {
						if (this.readyState === 4 && this.status === 200) {
							alert(this.responseText);
						}
					};
					x.send(this.files[0]);
					this.value = '';
				}
			});
		</script>
	</body>
</html>`);
	} else {
		res.writeHead(405);
		res.end();
	}
}

choose a file bigger then 100k you'll see http1 returns 405 immediately, but http2 never return any thing.
@apapirovski
Copy link
Member

I can't replicate this. I used the test script, after running https://localhost:1234 and entering a file, I get a 205 response to my PUT request immediately.

@xxoo
Copy link
Author

xxoo commented Apr 16, 2018

@apapirovski please try to use a file bigger then 100k

@apapirovski
Copy link
Member

Thanks. Can replicate now. Looks like it's likely tied to windowSize or something similar. Will investigate.

@apapirovski apapirovski added the confirmed-bug Issues with confirmed bugs. label Apr 16, 2018
@apapirovski
Copy link
Member

Further info: works on 9.3.0 and doesn't work on 9.4.0

@apapirovski
Copy link
Member

/cc @nodejs/http2 @addaleax

@addaleax addaleax added the http2 Issues or PRs related to the http2 subsystem. label Apr 16, 2018
@addaleax
Copy link
Member

Slightly reduced test case for what is probably the same issue, currently failing with NGHTTP2_ENHANCE_YOUR_CALM (but works on 9.3.0):

'use strict';
const http2 = require('http2');
const server = http2.createSecureServer({
  cert: `-----BEGIN CERTIFICATE-----
MIICpDCCAYwCCQDchnaEFAYYfzANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls
b2NhbGhvc3QwHhcNMTcxMTExMjA1NjQ2WhcNMTcxMjExMjA1NjQ2WjAUMRIwEAYD
VQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDP
CdzfoRXMW4rHYSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAq
mJfsiN1GRfMmO+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb
+vAYECKwWB9WJvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFy
dTnE2OOSx98ZG/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs
2dDrclBdw1gjIFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiL
uj3B31JYCCpr/fFWpuJ9AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAExF+pAYc0Kq
ejSiNxu4Ae5T5XLXOCFSGHRjHwE/Wpud0SqJIrLAAm6VfyU/MDvEIUlwfnHaHpyL
/j+1JNCLlMPcCY81k3CM3FrgnYit9ImU9ni3DU+c23zdMrkiF1bgQZBdbg2gzqgy
8yq2Ws72a6i0S8odCQ4/inCall3D9c64sefE9LocLTEBrTQDYp+bg6+RX7QKdmE6
6dkYm4oQOeB1lb0IdV+YSKz/bkypFPA0KY5dtpsoPK8TpwwUzg9cbnvXo0gKMbR3
qY8JmgsajfMbbynqR88kFHbtHOISC/XtmlObxSA+CqCKrI+f9ljc+wADPkF+rqxX
Gs1T2qkuJ6g=
-----END CERTIFICATE-----`,
  key: `-----BEGIN PRIVATE KEY-----
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDPCdzfoRXMW4rH
YSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAqmJfsiN1GRfMm
O+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb+vAYECKwWB9W
JvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFydTnE2OOSx98Z
G/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs2dDrclBdw1gj
IFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiLuj3B31JYCCpr
/fFWpuJ9AgMBAAECggEABZxO0ACdhpw0dpK7ZE3uiXEU3McFH4jq332JUvmbrLNN
PCns1w8EbCLsIhMCr9Ogu4bHFzHeTTk4DaEyECZK2ky5+5u8pVBlDaODF2unvWIn
YhmNHrIS5bGA28PB4ErYl12FhCFrzzuUHdGQqR1Vicb5U7I3MpvnOq6BKJGbwN3J
ZyEhlZ9MirbAQa12yv5h3827RkLeFXqzmxMHzKgT8VXbQwenZ3HzhwZ5jNd65g6M
yhBQw9tJvwjapm/gj66DpVdV9gJhzi5TBdRsIe5yu+QVInN0FKPCH7mpIfy3I+j4
0uYIXr/DLjv6sok6zQqvICQrhRwCQ557qsmGc5V8AQKBgQD/H/+iKzpqNgHYrgmF
ROPlb8bAVFslqKjKWiH7q6wUV2htQ6+S9giWxbmUXb5j2qG26YaEtPIcm+g5LEXm
Ox9MQjhNEeQIhb6KH5ghHrIDxqQNT2+sR49VbR3Y+OL6oBxFOv00nwg/h8KBK9NN
/asR15NmCka36OMwVinlr1xujQKBgQDPv6TcBuXF5b77h3OYoURnXv2ZJvULIDTV
pKaJa9Z/k8bCiTiHbKNjVGsfDijXEd82Dp6YwJMbYIAlexewR48vpQMOnGOkA9Hk
emMBfMy0t4i+8tZTT0UdYgVICsbZQSO/8L0LcEkGVfmRgJPrUhDw9o9AeD2YLnXM
7d9mBCD/sQKBgQDxhf6BLQFpKWXIFsLGmrhRLedvjqyXUzswDfIcCqKmwzUGM8zU
iP0Kl3cfwTuL1p+/xQZnPdHzSZmn/oTR9+iiThJ0y9ogQ1Vl95ES0bdfIb+PJkOn
SjukeN+H198xuz/oPncVSPULB+AYXz/0lpBMHNTbBiF63AuwZ/HUEpajxQKBgH8f
z1rQYbQaZSZ3eVXxgPEcYGRiQVpgh9Qf38SBl40TuXF7FHtSEB0NIEutl3IbvpHO
ml/wn1QGVgQZcaJt94F5IQjEy/gmWj7MYV8cpgsDsArggCQUgr97Jq4x4gI5aQ3f
215vhE/7Ni9CFcHOww0gYwJZUZ+Y9n7DJIvBhQvRAoGBAP68uVfOB7oVV3z9K6G+
oJHntre6Za8deemnksM/5H4/k4Kedl+RIMExhLsAypaP0aIZPW29Mwv3eT19ciUm
g6KrdhUNWvbNhryyg//VYnDLQi0FnGr+oZDQNhGxNdWXjxOBBTVxe4Z16CEDgIXI
Pwz9a9ZgvBd2si5/cMwM6aOe
-----END PRIVATE KEY-----`
}, onmessage).listen(1234);

function onmessage(req, res){
  res.writeHead(200);
  res.end('finished');
}

const client = http2.connect('https://localhost', { port: 1234, rejectUnauthorized: false });
const req = client.request({ ':method': 'PUT' }, { endStream: false });
req.on('response', () => {
  req.resume();
  req.on('end', () => {
    console.log('done!');
    client.destroy();
    server.close();
  });
});
req.end(Buffer.alloc(16 * 1024 * 1024));

@addaleax
Copy link
Member

Hm – I might be wrong about that. Bisecting points to 865da60, and adjusting maxSessionMemory on the client side seems to fix that, so it’s probably not really what’s happening here.

@apapirovski
Copy link
Member

This example does it:

'use strict';

const http2 = require('http2');
const server = http2.createSecureServer({
  cert: `-----BEGIN CERTIFICATE-----
MIICpDCCAYwCCQDchnaEFAYYfzANBgkqhkiG9w0BAQsFADAUMRIwEAYDVQQDDAls
b2NhbGhvc3QwHhcNMTcxMTExMjA1NjQ2WhcNMTcxMjExMjA1NjQ2WjAUMRIwEAYD
VQQDDAlsb2NhbGhvc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQDP
CdzfoRXMW4rHYSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAq
mJfsiN1GRfMmO+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb
+vAYECKwWB9WJvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFy
dTnE2OOSx98ZG/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs
2dDrclBdw1gjIFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiL
uj3B31JYCCpr/fFWpuJ9AgMBAAEwDQYJKoZIhvcNAQELBQADggEBAExF+pAYc0Kq
ejSiNxu4Ae5T5XLXOCFSGHRjHwE/Wpud0SqJIrLAAm6VfyU/MDvEIUlwfnHaHpyL
/j+1JNCLlMPcCY81k3CM3FrgnYit9ImU9ni3DU+c23zdMrkiF1bgQZBdbg2gzqgy
8yq2Ws72a6i0S8odCQ4/inCall3D9c64sefE9LocLTEBrTQDYp+bg6+RX7QKdmE6
6dkYm4oQOeB1lb0IdV+YSKz/bkypFPA0KY5dtpsoPK8TpwwUzg9cbnvXo0gKMbR3
qY8JmgsajfMbbynqR88kFHbtHOISC/XtmlObxSA+CqCKrI+f9ljc+wADPkF+rqxX
Gs1T2qkuJ6g=
-----END CERTIFICATE-----`,
  key: `-----BEGIN PRIVATE KEY-----
MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDPCdzfoRXMW4rH
YSXqR+exh9PfHkGFe7wF0yT5gPVgt6FdSMsYxbDjE6R3zfKSNSAqmJfsiN1GRfMm
O+tv6Ddvy5J/q9tAJmUxqsWcpe7aOAnWnwXNJSFoawjU2HPmKHzb+vAYECKwWB9W
JvztQrBN6WJnJCo1ffVq6qTEb1NiP0CXW/J8DgymmzP7gKTiSnFydTnE2OOSx98Z
G/YORGTX1w58mzFRkeIG1BAm3NB6jN4RGNaQoIyAPfInutlUbBSs2dDrclBdw1gj
IFGmPtEL15zEqhxuckCSkM1cJWCnm805ZoqNB7/PxpXrPVOVttiLuj3B31JYCCpr
/fFWpuJ9AgMBAAECggEABZxO0ACdhpw0dpK7ZE3uiXEU3McFH4jq332JUvmbrLNN
PCns1w8EbCLsIhMCr9Ogu4bHFzHeTTk4DaEyECZK2ky5+5u8pVBlDaODF2unvWIn
YhmNHrIS5bGA28PB4ErYl12FhCFrzzuUHdGQqR1Vicb5U7I3MpvnOq6BKJGbwN3J
ZyEhlZ9MirbAQa12yv5h3827RkLeFXqzmxMHzKgT8VXbQwenZ3HzhwZ5jNd65g6M
yhBQw9tJvwjapm/gj66DpVdV9gJhzi5TBdRsIe5yu+QVInN0FKPCH7mpIfy3I+j4
0uYIXr/DLjv6sok6zQqvICQrhRwCQ557qsmGc5V8AQKBgQD/H/+iKzpqNgHYrgmF
ROPlb8bAVFslqKjKWiH7q6wUV2htQ6+S9giWxbmUXb5j2qG26YaEtPIcm+g5LEXm
Ox9MQjhNEeQIhb6KH5ghHrIDxqQNT2+sR49VbR3Y+OL6oBxFOv00nwg/h8KBK9NN
/asR15NmCka36OMwVinlr1xujQKBgQDPv6TcBuXF5b77h3OYoURnXv2ZJvULIDTV
pKaJa9Z/k8bCiTiHbKNjVGsfDijXEd82Dp6YwJMbYIAlexewR48vpQMOnGOkA9Hk
emMBfMy0t4i+8tZTT0UdYgVICsbZQSO/8L0LcEkGVfmRgJPrUhDw9o9AeD2YLnXM
7d9mBCD/sQKBgQDxhf6BLQFpKWXIFsLGmrhRLedvjqyXUzswDfIcCqKmwzUGM8zU
iP0Kl3cfwTuL1p+/xQZnPdHzSZmn/oTR9+iiThJ0y9ogQ1Vl95ES0bdfIb+PJkOn
SjukeN+H198xuz/oPncVSPULB+AYXz/0lpBMHNTbBiF63AuwZ/HUEpajxQKBgH8f
z1rQYbQaZSZ3eVXxgPEcYGRiQVpgh9Qf38SBl40TuXF7FHtSEB0NIEutl3IbvpHO
ml/wn1QGVgQZcaJt94F5IQjEy/gmWj7MYV8cpgsDsArggCQUgr97Jq4x4gI5aQ3f
215vhE/7Ni9CFcHOww0gYwJZUZ+Y9n7DJIvBhQvRAoGBAP68uVfOB7oVV3z9K6G+
oJHntre6Za8deemnksM/5H4/k4Kedl+RIMExhLsAypaP0aIZPW29Mwv3eT19ciUm
g6KrdhUNWvbNhryyg//VYnDLQi0FnGr+oZDQNhGxNdWXjxOBBTVxe4Z16CEDgIXI
Pwz9a9ZgvBd2si5/cMwM6aOe
-----END PRIVATE KEY-----`
}, onmessage).listen(1234);

function onmessage(req, res) {
  res.writeHead(200);
  res.end('finished');
}

const client = http2.connect('https://localhost', { port: 1234, rejectUnauthorized: false });
const req = client.request({ ':method': 'PUT' }, { endStream: false });
req.on('response', () => {
  req.resume();
  req.on('end', () => {
    console.log('done!');
    client.close();
    server.close();
  });
});
req.end(Buffer.alloc(16 * 16 * 1024));

@addaleax
Copy link
Member

@apapirovski Not sure – client.close() should be client.destroy(), right?

@apapirovski
Copy link
Member

@addaleax I don't think so. close is more similar to what a browser would do in this situation. destroy just shuts down the connection whether or not any streams are still open.

The alternative would be:

req.on('finish', () => {
    console.log('done!');
    client.destroy();
    server.close();
  });

Basically, only destroy the connection after the finish event which will never fire, even though it should.

@apapirovski
Copy link
Member

apapirovski commented Apr 16, 2018 •
edited
Loading

I suppose if you want to test this against 9.3.0 then my change above would be better.

@apapirovski
Copy link
Member

apapirovski commented Apr 16, 2018 •
edited
Loading

I think this code:

node/lib/internal/http2/core.js

Lines 1864 to 1879 in 809eb27

[kMaybeDestroy](error, code = NGHTTP2_NO_ERROR) {
if (error || code !== NGHTTP2_NO_ERROR) {
this.destroy(error);
return;
}
// TODO(mcollina): remove usage of _*State properties
if (this._readableState.ended &&
this._writableState.ended &&
this._writableState.pendingcb === 0 &&
this.closed) {
this.destroy();
// This should return, but eslint complains.
// return
}
}

needs to be checking for whether the user has done any consuming on the readable side and if they haven't, then just proceeding to destroy. Which then will call .close(0) on the stream.

That said, as far as I know, that requires manually tracking whether the stream has been consumed in _read and also checking if a resume is currently scheduled. Not exactly clean...

We do something similar in http2/compat.js with didRead if anyone wants to dig further.

Edit: Or maybe it shouldn't destroy but just call resume like http and http2/compat.js do... I don't really know the answer. (Although resume seems wasteful since it would consume the entirety of the incoming payload.)

@mcollina
Copy link
Member

Edit: Or maybe it shouldn't destroy but just call resume like http and http2/compat.js do... I don't really know the answer. (Although resume seems wasteful since it would consume the entirety of the incoming payload.)

I think this might be a good thing to do.

IMHO this is connected to #19852 as well. (it might need some help to bring it over the line, it makes a lot of tests flaky).

@apapirovski
Copy link
Member

Think I might have the fix. It's not pretty... but maybe others can help once I open the PR. 😄

@apapirovski apapirovski mentioned this issue Apr 16, 2018
Closed
3 tasks
MylesBorins pushed a commit that referenced this issue May 4, 2018
@apapirovski @MylesBorins
http2: fix responses to long payload reqs
46bd862 
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

PR-URL: #20084
Fixes: #20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@xxoo
Copy link
Author

xxoo commented May 9, 2018 •
edited
Loading

@apapirovski i've tried 10.1.0 with the first example in chrome, but seem the problem is still there.

@apapirovski apapirovski reopened this May 9, 2018
@apapirovski
Copy link
Member

apapirovski commented May 9, 2018 •
edited
Loading

Ok, so looks like the underlying bug was fixed and it works with the new duplex stream but the compatibility mode still doesn't. Looking into it.

Edit: this is tied to the new Trailers implementation which was developed during the same time that PR happened.

@apapirovski apapirovski mentioned this issue May 9, 2018
Closed
3 tasks
@apapirovski
Copy link
Member

@xxoo There's a fix in #20621. I've confirmed it works against the test case in the original.

@xxoo
Copy link
Author

xxoo commented May 10, 2018

@apapirovski good news! thanks man!

MylesBorins pushed a commit that referenced this issue May 22, 2018
@apapirovski @MylesBorins
http2: fix end without read
b2fb1d7 
Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

PR-URL: #20621
Fixes: #20060
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Aug 23, 2018
@apapirovski @kjin
http2: fix responses to long payload reqs
eb18c26 
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

PR-URL: nodejs#20084
Fixes: nodejs#20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Aug 23, 2018
@apapirovski @kjin
http2: fix end without read
663191e 
Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

PR-URL: nodejs#20621
Fixes: nodejs#20060
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Sep 19, 2018
@apapirovski @kjin
http2: fix responses to long payload reqs
bbc3bfd 
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

PR-URL: nodejs#20084
Fixes: nodejs#20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Sep 19, 2018
@apapirovski @kjin
http2: fix end without read
c27e2b8 
Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

PR-URL: nodejs#20621
Fixes: nodejs#20060
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
@apapirovski @kjin
http2: fix responses to long payload reqs
7bb1a8c 
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

PR-URL: nodejs#20084
Fixes: nodejs#20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue Oct 16, 2018
@apapirovski @kjin
http2: fix end without read
2238e12 
Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

PR-URL: nodejs#20621
Fixes: nodejs#20060
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
@apapirovski @BethGriggs
http2: fix responses to long payload reqs
41dca9e 
When a request with a long payload is received, http2 does
not allow a response that does not process all the incoming
payload. Add a conditional Http2Stream.close call that runs
only if the user hasn't attempted to read the stream.

Backport-PR-URL: #22850
PR-URL: #20084
Fixes: #20060
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
BethGriggs pushed a commit that referenced this issue Oct 17, 2018
@apapirovski @BethGriggs
http2: fix end without read
b0c92ca 
Adjust http2 behaviour to allow ending a stream even after some
data comes in (when the user has no intention of reading that
data). Also correctly end a stream when trailers are present.

Backport-PR-URL: #22850
PR-URL: #20621
Fixes: #20060
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
confirmed-bug Issues with confirmed bugs. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
4 participants
@apapirovski @mcollina @addaleax @xxoo