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

Response in ended before the catch in RequestListener #13

Closed
asadhazara opened this issue Dec 31, 2021 · 7 comments · Fixed by #14
Closed

Response in ended before the catch in RequestListener #13

asadhazara opened this issue Dec 31, 2021 · 7 comments · Fixed by #14
Labels
question Further information is requested released Has been released and published

Comments

@asadhazara
Copy link
Contributor

asadhazara commented Dec 31, 2021

In the following example the server is not returning 500 Internal Server Error. Instead the server throws an error (Error [ERR_HTTP_HEADERS_SENT]: Cannot render headers after they are sent to the client) and the process is killed.

import http from 'http';
import { createHandler } from 'graphql-sse';

const handler = createHandler({
  ...
  onNext: () => {
    throw new Error('Not implemented');
  },
});

http.createServer(async (req, res) => {
  try {
    await handler(req, res);
  } catch (err) {
    res.writeHead(500, 'Internal Server Error').end();
  }
});
@enisdenjo
Copy link
Owner

enisdenjo commented Dec 31, 2021

Hmm, I couldn't manage to replicate your issue, I've added tests describing your case, and they pass.

Can you elaborate further? Which version are you using? How are you connecting to the server? Which client are you using?

Ideally a full repro would be perfect and much appreciated!

@enisdenjo
Copy link
Owner

enisdenjo commented Dec 31, 2021

Ah, ok, I misread - the error is thrown properly, but you have problems writing the headers.

Writing a 200: OK is necessary in order to establish an SSE connection; meaning, headers are already written and flushed at the point where onNext is reached because there's an active and accepted SSE connection.

@enisdenjo enisdenjo added the question Further information is requested label Dec 31, 2021
@asadhazara
Copy link
Contributor Author

asadhazara commented Dec 31, 2021

If I want to write a different head, what are the options? For instance, I would like to send a 500 when something goes wrong in a resolver.

@enisdenjo
Copy link
Owner

You cannot write a different header. As mentioned before, it is already written and flushed at that point.

You can only leave a log on your server and fix the onNext handler.

@enisdenjo
Copy link
Owner

enisdenjo commented Dec 31, 2021

Since re-writing the header is not possible, you can catch the error in onNext and populate the errors GraphQL result field to transmit to the client through the next message.

Something around the edges:

import http from 'http';
import { createHandler } from 'graphql-sse';

const handler = createHandler({
  // ... your other options ...
  onNext: () => {
    try {
      mightThrowAnError();
    } catch (err) {
      return {
        errors: [new GraphQLError(err)],
      };
    }
  },
});

http.createServer(async (req, res) => {
  try {
    await handler(req, res);
  } catch (err) {
    if (!res.headersSent) {
      res.writeHead(500, 'Internal Server Error').end();
    } else {
      console.error('Internal Server Error', err);
    }
  }
});

But beware, I do NOT recommend this. You should absolutely make sure that nothing is thrown from onNext. Execution errors, errors that are a part of the next message, are not terminal - the client will not end and the operation will proceed blindly.

@asadhazara
Copy link
Contributor Author

Thank you @enisdenjo! The example I have used is directly copied from the JSDoc for the Handler. I have submitted a PR for this.

@enisdenjo
Copy link
Owner

🎉 This issue has been resolved in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@enisdenjo enisdenjo added the released Has been released and published label Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested released Has been released and published
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants