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

Return meaningful errors #133

Closed
5 tasks
lidel opened this issue Mar 20, 2024 · 4 comments · Fixed by #195
Closed
5 tasks

Return meaningful errors #133

lidel opened this issue Mar 20, 2024 · 4 comments · Fixed by #195
Assignees
Milestone

Comments

@lidel
Copy link
Member

lidel commented Mar 20, 2024

iiuc, right now, we seem to return error that produces default error from browser engine:

https://github.com/ipfs-shipyard/helia-service-worker-gateway/blob/41d2cffcff5bc3061212fe13aadb5df9c1a3f64c/src/sw.ts#L389

https://github.com/ipfs-shipyard/helia-service-worker-gateway/blob/41d2cffcff5bc3061212fe13aadb5df9c1a3f64c/src/sw.ts#L387

This makes it hard for user to reason what went wrong, and near to impossible to fill a meaningful error report.

Example: failure to fetch block during path walk

If you walk https://inbrowser.dev/ipns/en.wikipedia-on-ipfs.org long enough, you may hit a hiccup:

2024-04-11-171819_658x702_scrot

Proposed improvement

If we encounter an error while generating a Response within a service worker, we should always catch the error and respond with a meaningful HTML error page.

  • return text/html
  • human-readable
  • include error details (short message, full JS error's stack)
  • do this for timeouts and redirects as well
  • VERY IMPORTANT UX: have "golden path" for fixing hiccups. HTML big green button "Retry" that allows user to reload page and try again

Pseudocode

async function handleRequest(request) {
  try {
    const response = await fetch(request)
    return response
  } catch (error) {
    // If an error occurs, respond with a custom error page including error details
    const errorResponse = new Response(`
      <h1>Oops! Something went wrong inside of Service Worker IPFS Gateway.</h1>
      <p><button onclick="window.location.reload(true);">🔃 Click here to retry</button>.</p>
      <p>Error details:</p>
      <p>${error.message}</p>
      <pre>${error.stack}</pre>
    `, {
      status: 500,
      statusText: 'Internal Service Worker Gateway Error',
      headers: new Headers({
        'Content-Type': 'text/html'
      })
    })
    return errorResponse
  }
}
@SgtPooki SgtPooki added this to the Beta milestone Apr 8, 2024
@lidel lidel modified the milestones: Beta, Alpha Apr 9, 2024
@lidel lidel changed the title Return Meaningful HTML errors Return meaningful errors Apr 11, 2024
@SgtPooki SgtPooki linked a pull request Apr 13, 2024 that will close this issue
3 tasks
@SgtPooki
Copy link
Member

IIRC, the body is checked in gateway conformance tests for some failure cases, i.e. https://github.com/ipfs/gateway-conformance/blob/9beb71f432a1e83f2dbc10031612991570ef5397/tests/path_gateway_dag_test.go#L371

We will need some sophisticated handling in verified-fetch to determine whether to surface compliance required error messaging vs error stack traces and whatnot in order to render a useful page here.

Do we want to return this error page HTML from @helia/verified-fetch ?

@lidel
Copy link
Member Author

lidel commented Apr 17, 2024

Good catch, the conformance test you linked is too specific,
should be relaxed to only checking for HTTP 510 status code – opened ipfs/gateway-conformance#205, feel free to skip these two for now.

This allows us to separate concerns, and have branded HTML in service worker:

  • verified fetch throws or returns text/plain errors (we don't need to implement HTML errors here)
  • service worker gateway catches or wraps them in text/html (see example snipped I posted above)
    • we could limit it to requests for the top level document, and keep plaintext errors for subresources to make debugging easier. (test could be something like const isTopLevelDocumentRequest = request.mode === 'navigate' && request.destination === 'document')

@SgtPooki
Copy link
Member

I think verified-fetch will need to return application/json errors in Responses (not text/plain) for us to get a useful stacktrace here.

@lidel
Copy link
Member Author

lidel commented Apr 19, 2024

application/json with stacktrace would be nice, but I think just concatenating err in message returned in return badGatewayResponse(resource, 'Error walking path') will be huge improvement. Right now we lose the meaningful error.

@SgtPooki SgtPooki self-assigned this Apr 23, 2024
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

Successfully merging a pull request may close this issue.

2 participants