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

[BUG] BrokenPipe Crash #102

Closed
0xtlt opened this issue Jan 8, 2021 · 7 comments · Fixed by #135
Closed

[BUG] BrokenPipe Crash #102

0xtlt opened this issue Jan 8, 2021 · 7 comments · Fixed by #135
Labels
bug Something isn't working

Comments

@0xtlt
Copy link
Contributor

0xtlt commented Jan 8, 2021

Issue

Setup:

deno 1.6.3 (release, x86_64-unknown-linux-gnu)
v8 8.8.294
typescript 4.1.3

  • Opine Version: 1.0.0

Uncaught BrokenPipe: Broken pipe (os error 32)

Details

Hello, it's been a while since Opine crashed randomly with a system error, at the time it was on my mac, now on my windows and also on a linux server, the error returned is still the same:

error: Uncaught (in promise) BrokenPipe: Broken pipe (os error 32)
    at unwrapResponse (deno:runtime/js/10_dispatch_minimal.js:59:13)
    at sendAsync (deno:runtime/js/10_dispatch_minimal.js:98:12)
    at async write (deno:runtime/js/12_io.js:117:20)
    at async BufWriter.write (bufio.ts:499:29)
    at async writeResponse (_io.ts:273:15)
    at async Proxy.respond (server.ts:84:7)
    at async Response.end (response.ts:256:7)

Maybe you have to surround Opine with a try catch?
The crash occurs with any code that uses opine, however I noticed that the more routes there are, the more often it happens

@asos-craigmorten
Copy link
Collaborator

Hey @techtastet 👋

So Opine is designed currently to catch some errors that are thrown from Deno's Http std lib as you can see here --> https://github.com/asos-craigmorten/opine/blob/main/src/response.ts#L256, namely instances of Deno.errors.BadResource as this is an indicator that the connection has already been closed.

Other than that, the responsibility of error handling lies with your code to add appropriate error handling, either by adding an error middleware to your application, or adding catchs around your middleware code, e.g.

try {
  await res.end();
} catch(e) {
  // ... do something with the error.
}

Check out the error examples and the Error-handling middleware section of the API docs.

Let me know if need further assistance. If you believe it is an issue with Opine, please provide a reproduce-able code example 😄

@asos-craigmorten asos-craigmorten added the needs information More information is required before this can be actioned label Jan 8, 2021
@cmorten
Copy link
Owner

cmorten commented Jan 17, 2021

This appears to originate from a bug in the std library which has since been fixed. See:

And was fixed in: denoland/deno#8365

Please re-open if you are finding it still happens and provide a reproduceable example 😄

@cmorten cmorten closed this as completed Jan 17, 2021
@TheAifam5
Copy link

The issue still exist and it happens when the request is cancelled. I use OpineHttpProxy in this case too.

@Tc-001
Copy link

Tc-001 commented Jul 23, 2021

It seems that deno also can throw Deno.errors.BrokenPipe if a connection is closed unexpectedly (denoland/std#659 (comment)), and it is not being handeled in:
https://github.com/asos-craigmorten/opine/blob/fe9adf5f82703ab464adb3aecf29b05f542ce2f8/src/response.ts#L266-L273
Thus resulting in a server crash

@asos-craigmorten
Copy link
Collaborator

asos-craigmorten commented Jul 23, 2021

Yeah, looks like closed this in error. We will need to handle Deno.errors.BrokenPipe errors emitted when calling the .next() on the server iterator.

TBC if can wrap the response

https://github.com/asos-craigmorten/opine/blob/fe9adf5f82703ab464adb3aecf29b05f542ce2f8/src/response.ts#L256-L263

or if need to catch at the top-level loop

https://github.com/asos-craigmorten/opine/blob/fe9adf5f82703ab464adb3aecf29b05f542ce2f8/src/application.ts#L569-L571

It sounds like we would benefit from moving from the async interator to a while loop where call .next() manually, e.g. something similar to (not tested)...

// ... rest of code

while (!closed) {
  try {
    const request = await server.next();
  } catch (e) {
    // handle Deno.errors.BrokenPipe and other errors
  }

  this(request as Request);

  // ... rest of code
}

// ... rest of code

@asos-craigmorten asos-craigmorten added bug Something isn't working and removed needs information More information is required before this can be actioned labels Jul 23, 2021
@asos-craigmorten asos-craigmorten changed the title BrokenPipe Crash [BUG] BrokenPipe Crash Jul 23, 2021
@Tc-001
Copy link

Tc-001 commented Jul 23, 2021

Thanks for the fast reply!

For future reference, I could reproduce it by

app.get("/",  function(req, res) {
  res.send("a ".repeat(9999999));
});

and spamming F5 in firefox.
This cancels the previous load before re-requesting the page. There is probably a better way, but this worked in a pinch.

@asos-craigmorten
Copy link
Collaborator

Ah nice @Tc-001 that will be useful for repro + figuring out if fix. Can probably add a "racing server" like integration test similar to what the std/http module has for these kind of edge cases in real world apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants