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

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

Closed
btargac opened this issue Mar 8, 2021 · 11 comments
Closed

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

btargac opened this issue Mar 8, 2021 · 11 comments

Comments

@btargac
Copy link

btargac commented Mar 8, 2021

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
Copy link
Owner

Hello @btargac

So to recap :

  • when useElectronNet: true you cannot read the content-type header
  • when useElectronNet: false you can read the content-type header

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
Copy link
Author

btargac commented Mar 8, 2021

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
Copy link
Owner

@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
Copy link
Author

btargac commented Mar 8, 2021

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
Copy link
Owner

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
Copy link
Author

btargac commented Mar 9, 2021

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
Copy link
Owner

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 : electron/electron#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
Copy link
Owner

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

@btargac
Copy link
Author

btargac commented Mar 11, 2021

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
Copy link
Owner

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
Copy link
Author

btargac commented Mar 12, 2021

😄 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants