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

Problems with getReadStream in updated version 2.8.1 #795

Closed
huineng opened this issue Jan 12, 2023 · 6 comments
Closed

Problems with getReadStream in updated version 2.8.1 #795

huineng opened this issue Jan 12, 2023 · 6 comments
Assignees
Labels

Comments

@huineng
Copy link

huineng commented Jan 12, 2023

Hi, this relates to the pull request leading to the latest version 2.8.1 #790

Since i installed this version (my previous version was 2.8.0) i have problems with the updated getReadStream function

The code below was until 2.8.1 perfectly running and downloading requested files

const stream = await box.files.getReadStream(fileId);

stream.pipe(res);

return new Promise((reject, resolve) => {
    stream.on('error', reject);
    stream.on('finish', resolve);
});

Since version 2.8.1 i can still download the file but now i also get unhandled rejections

{
  message: Promise {
    <rejected> Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
        at new NodeError (node:internal/errors:400:5)
        at ServerResponse.setHeader (node:_http_outgoing:663:11)
        at ServerResponse.header /node_modules/express/lib/response.js:794:10)
        at ServerResponse.json (/node_modules/express/lib/response.js:275:10)
        at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
      code: 'ERR_HTTP_HEADERS_SENT'
    }
  },
  name: 'unhandledrejection',
  reason: Error [ERR_HTTP_HEADERS_SENT]: Cannot set headers after they are sent to the client
      at new NodeError (node:internal/errors:400:5)
      at ServerResponse.setHeader (node:_http_outgoing:663:11)
      at ServerResponse.header (/node_modules/express/lib/response.js:794:10)
      at ServerResponse.json (/node_modules/express/lib/response.js:275:10)
      at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
    code: 'ERR_HTTP_HEADERS_SENT'
  },
}

I there something in the code above i need to change to support your changes , or is this a regression bug ?

thanks

@congminh1254
Copy link
Member

Hi @huineng,

Thanks for submitting this issue, we are checking it and will get back to you ASAP.

Thanks for your patience.

Minh

@congminh1254
Copy link
Member

congminh1254 commented Jan 12, 2023

Hi @huineng,

I just tried with this following code and it run without any error, can you check again and provide some more information about runtime environment (Node version, network condition)

let length = 0;

async function fetch() {
	let stream = await client.files.getReadStream('123456789');
	var res = new fs.WriteStream('test.docx');
	stream.pipe(res);

	return new Promise((resolve, reject) => {
		stream.on('data', (chunk) => {
			length += chunk.length;
		});
		stream.on('error', reject);
		stream.on('finish', resolve);
	});
}

fetch()
	.then(() => {
		console.log('Finished, received ' + length + ' bytes');
	})
	.catch((err) => {
		console.log(err);
		throw err;
	});

Output:

Finished, received 10464 bytes

We are looking for your response.

Thankyou 😄

@huineng
Copy link
Author

huineng commented Jan 13, 2023

thanks for your fast answer. The difference here is that i'm using this in an express application as part of a rest api where users can download a file (hence the specific express errors) box is mostly used in rest file download context

below is a small code (in typescript running ts-node) where i have the same problem
2.8.1 has express headers problem
2.8.0 doesn't have any problem

node 18.13.0
box-node-sdk: 2.8.0 <-> 2.8.1
express: 4.18.2

import express from 'express';
import BoxSDK from 'box-node-sdk';
import * as http from 'http';

const sdk = new BoxSDK(credentials);
const client = sdk.getAppAuthClient('enterprise', credentials.enterpriseID);

const fileDownload = async (res: Express.Response) => {
  const stream = await client.files.getReadStream('1234567');

  stream.pipe(res);

  return new Promise((reject, resolve) => {
    stream.on('error', reject);
    stream.on('finish', resolve);
  });
};

const app = express();

app.get('/file', async (_req, res) => {
  try {
    const file = await fileDownload(res);
    return res.status(200).send(file);
  } catch (err) {
    return res.status(200).json(err);
  }
});

app.set('port', 8192);

const httpServer = http.createServer(app);

httpServer.listen(app.get('port'), async () => {
  console.info('server started');
});

thanks

ps this is the console log of 2.8.1

Error: Cannot set headers after they are sent to the client
at new NodeError (node:internal/errors:400:5)
at ServerResponse.setHeader (node:_http_outgoing:663:11)
at ServerResponse.header (/node_modules/express/lib/response.js:794:10)
at ServerResponse.json (/node_modules/express/lib/response.js:275:10)
at /index.ts:43:28
at processTicksAndRejections (node:internal/process/task_queues:95:5)

@congminh1254
Copy link
Member

Hi @huineng,

I am getting back to you with a good news 😄, you have a mistake in your code, so the Promise should be Promise((resolve, reject) => { }), but not (reject, resolve).

I hope fixing this in your code it will also close this issue.

Best,
Minh Nguyen

@congminh1254
Copy link
Member

And I got some more information about the error message Error: Cannot set headers after they are sent to the client, because in this part:

  try {
    const file = await fileDownload(res);
    return res.status(200).send(file);
  } catch (err) {
    return res.status(200).json(err);
  }

As the file has already been written into the response stream, if any errors are raised, it will trigger res.status(200)... and try to write another header to the response stream, at which point the library express will raise the error message to prevent writing headers after the body has been written.

I hope this can help.

Best,
Minh Nguyen

@huineng
Copy link
Author

huineng commented Jan 13, 2023

Ok, thank you very much .. it got me puzzled why it was working before

The reject resolve piece i never paid really attention too (i think i copied it from somewhere) because it simply worked
in fact i could have simply done return new Promise(() => { }) and it would still have worked..

The update changed all of that and now i was faced with the incorrect reject resolve
I had to make a few other small changes, but now it all works.. so i can close this ticket

Many thanks

@huineng huineng closed this as completed Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants