-
Notifications
You must be signed in to change notification settings - Fork 30k
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
difference between http2 Compatibility API and http #20060
Comments
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. |
@apapirovski please try to use a file bigger then 100k |
Thanks. Can replicate now. Looks like it's likely tied to windowSize or something similar. Will investigate. |
Further info: works on 9.3.0 and doesn't work on 9.4.0 |
/cc @nodejs/http2 @addaleax |
Slightly reduced test case for what is probably the same issue, currently failing with '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)); |
Hm – I might be wrong about that. Bisecting points to 865da60, and adjusting |
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)); |
@apapirovski Not sure – |
@addaleax I don't think so. The alternative would be: req.on('finish', () => {
console.log('done!');
client.destroy();
server.close();
}); Basically, only destroy the connection after the |
I suppose if you want to test this against 9.3.0 then my change above would be better. |
I think this code: node/lib/internal/http2/core.js Lines 1864 to 1879 in 809eb27
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 That said, as far as I know, that requires manually tracking whether the stream has been consumed in We do something similar in Edit: Or maybe it shouldn't destroy but just call |
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). |
Think I might have the fix. It's not pretty... but maybe others can help once I open the PR. 😄 |
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
@apapirovski i've tried 10.1.0 with the first example in chrome, but seem the problem is still there. |
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 good news! thanks man! |
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
choose a file bigger then 100k you'll see http1 returns 405 immediately, but http2 never return any thing.
The text was updated successfully, but these errors were encountered: