grantila / fetch-h2

HTTP/1+2 Fetch API client for Node.js
MIT License
336 stars 16 forks source link

multiple sync http2 requests lead to multiple connections instead of reusing #39

Closed cmawhorter closed 4 years ago

cmawhorter commented 5 years ago

i don't know enough about the fetch spec to say if this is a bug or not but it's definitely not what i expected.

this code below causes 10 separate connections. i'd expect what to happen in this situation is the first request blocks the others until the connection is established and they all share it.

additionally, this causes something to be off with a stuck reference or something. "complete" is printed but the process never exits.

const { fetch, disconnectAll } = require('fetch-h2');

const urls = [
'https://example.com/api/1',
'https://example.com/api/2',
'https://example.com/api/3',
'https://example.com/api/4',
'https://example.com/api/5',
'https://example.com/api/6',
'https://example.com/api/7',
'https://example.com/api/9',
'https://example.com/api/10',
];

const main = async () => {
    const results = await Promise.all(urls.map(async url => {
        const res = await fetch(url);
        const body = await res.text();
        return body;
    }));
    await disconnectAll();
    return 'complete';
};

main().then(console.log);

blocking with an initial call fixes issue:

const main = async () => {
  await fetch('https://example.com/');
  const results = await Promise.all(...

That results in just one connection being used and the process exiting as expected. It seems related to getOrCreateHttp2 but not sure. 😪 🛏

grantila commented 5 years ago

That's likely a bug, if the ALPN negotiation results in http2.

The flow is supposed to be that when an origin (in your case example.com is unknown to fetch-h2, ALPN negotiation ends up in either http1 or http2 and consecutive immediate requests to the same origin is blocked until the negotiation is complete. If negotiation results in http2, a single connection should be re-used. If http1, connection pooling will happen.

I'll look into this.

cmawhorter commented 5 years ago

Ah, makes sense. Everything works great otherwise. Loving it.

Btw - I think the stuck references not being cleared by disconnectAll is a different issue. I've been running into it separate from what I described above.

And totally unrelated, but what's stopping this from node 8 support? Have you looked into porting the node10 http2.js back or something? AWS lambda doesn't have v10 yet. I can get fetch-h2 mostly running under node 8 via rollup except for http2.

grantila commented 5 years ago

Yeah http1 probably works in Node 8, but since the library started with (only) http2, I don't want to claim it in the readme. http2 in Node 8 is really broken + the API has changed since then.

Anyway, it's pretty sad we don't have Node 10-support in AWS lambdas, shame on them ;) On the other hand, you can upload binaries in lambdas, e.g. Node 10 😆

github-actions[bot] commented 4 years ago

:tada: This issue has been resolved in version 2.4.3 :tada:

The release is available on:

Your semantic-release bot :package::rocket: