arantes555 / electron-fetch

A light-weight module that brings window.fetch to the background process of Electron
Other
133 stars 21 forks source link

can not read header content-type if useElectronNet is true #33

Closed btargac closed 3 years ago

btargac commented 3 years ago

Default behavior is setting useElectronNet to true as mentioned in documentation,

but in my app I cannot read the header information content-type.

My app downloads hundreds of files in chunks for example in 20 item groups then the next 20 one and so on, so I create a job stack and process them in a loop with awaiting,

when I set useElectronNet I can read the content-type so I am confused a little bit, am I doing sth wrong or is there an implementation problem with electron's net module ?

the below code might help with the issue;


const processItem = async (item, outputPath) => {

  const [ itemUrl, newName, subFolderName ] = item;
  const url = new URL(itemUrl);
  const itemName = newName ? `${newName}${path.extname(url.pathname)}` : path.basename(url.pathname);

  const response = await fetch(itemUrl, {useElectronNet: false});

  if (response.ok) {
    if (subFolderName) {
      await mkdir(`${outputPath}/${subFolderName}`, { recursive: true }, () => {});
    }

    const contentType = response.headers.get('content-type');
    const extension = mime.extension(contentType);
    console.log('extension', extension);

    const dest = createWriteStream(path.join(outputPath, subFolderName ? subFolderName : '', itemName));

    await streamPipeline(response.body, dest);
  } else {
    throw {
      status: response.status,
      statusText: response.statusText,
      itemInfo: url.href
    }
  }
};

any idea what I am missing or using electron's net is problematic in my apps electron version ?

Environment information;

electron": "^12.0.0", "electron-builder": "^22.10.5",

node: v14.15.1, npm: 6.14.8

arantes555 commented 3 years ago

Hello @btargac

So to recap :

Am I understanding you correctly?

If so, I really do not know where it may be coming from, as the tests extensively use content-type header, so I am quite sure it is correctly passed. From your code example, you don't seem to be doing anything "weird" that may cause this kind of issue. Also, I just updated the tests to try them on electron 12, and it seems to work correctly.

Maybe you can try on an older electron version ? It would not be the first time they break something in the net module with an update...

In any case, I would very much appreciate a snippet for a full repro case that I could try myself. I cannot do much more without reproducing the bug locally.

btargac commented 3 years ago

Hello @arantes555,

yep what you understood is totally right.

To be honest I was dealing something different but updated some dependencies as well, but haven't tried with an older version of electron.

If you are interested here is the full repo and branch to reproduce the case

https://github.com/btargac/excel-parser-processor/tree/feature/handle-extension-free-urls

the code sample is in src/utils/processItem.js and you will need a sample excel file to see the case here you can download a sample one https://docs.google.com/spreadsheets/d/1Gmy4PQ2d5mNLIirJDpVnfbF9Iiv_Xj6OiNUnuQidT20/edit?usp=sharing

arantes555 commented 3 years ago

@btargac Sorry but I will need a minimal reproduction snippet to be able to investigate. There are too many moving parts in a full application to isolate this specifically.

btargac commented 3 years ago

oops sorry for that,

I'll try to make a small set of minimum required code then, but I guess you'll still need to run that snippet with Electron runtime to reproduce not a node REPL etc, am I right ?

arantes555 commented 3 years ago

No worries

Yes, I'll need to run it with Electron, but that's quite easy to do :)

If you're not sure how to proceed, the "best" way for me would be that you fork the electron-fetch repo, and edit the tests.js file to add a test that fails with your issue.

btargac commented 3 years ago

Hi , I created a test scenario in the tests file, and tried to comment out what works well and what not, hope this helps to see the actual root cause, is it using two fetches without waiting the first one's stream finish etc ?

Here you can reach the test file

https://github.com/btargac/electron-fetch/blob/electron-net-test/test/test.js

arantes555 commented 3 years ago

Hello @btargac

After investigation, it seems that it's a bug in electron itself, in the net component : it happens only when the server does not actually respond with a 200 but with a 304 (which means the client already has the response in the cache, so the server does not send it again) : starting from v7, electron "magically" transforms the response from 304 to 200 with the content from the cache, but does not populate the headers correctly. (Electron v6 actually populates the headers as expected.)

Here is the corresponding electron issue : https://github.com/electron/electron/issues/27895

I will not be able to actually solve this on electron-fetch's side. However, you can work-around the issue by preventing the request from using the cache, by manually adding a null If-None-Match header :

await fetch(url, { useElectronNet: true, headers: { 'If-None-Match': null } })

Hope this helps.

I will close the issue here as the problem comes from upstream.

arantes555 commented 3 years ago

Also, don't hesitate to upvote the upstream issue if you want it fixed :)

btargac commented 3 years ago

Hi @arantes555,

thanks a lot for a deep dive, I'll go with your cache killer header approach, and will definitely upvote net module issue at electron issues 👍🏼

arantes555 commented 3 years ago

It's ironic, the issue has existed since electron 7, released in October 2019, and you report it to me only 2 weeks after someone else reports it to electron 🤷

btargac commented 3 years ago

😄 guess I'm lucky that my code or my dependencies are not problematic but my environment (electron itself in this case) is problematic, btw I was not dealing with headers directly just saving the file returned from server but from now on I should find the file extension from content-type, so just faced the problem while trying to a new feature.