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

feat: various fixes and improvements #53

Merged
merged 9 commits into from
Nov 8, 2023
Merged

feat: various fixes and improvements #53

merged 9 commits into from
Nov 8, 2023

Conversation

SgtPooki
Copy link
Member

@SgtPooki SgtPooki commented Nov 8, 2023

  • Deps update
  • added ./debugging scripts & readme to help with testing locally
  • swap fs.stat() async race for root files (index.html, etc) to fs.ls iterator.. seems much faster (and less wasteful)
  • listen for client aborted requests in fastify by passing a signal down through operations and aborting on client abort.
  • replace consistently failing e2e tests with more "smoketest" e2e tests.
  • Handle errors that were causing server to fall over when requests are aborted (see ab4a0f3)

Copy link
Member Author

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self review

@@ -36,6 +36,7 @@ $ docker run -it -p 8080:8080 -e DEBUG="helia-http-gateway" helia
| Variable | Description | Default |
| --- | --- | --- |
| `DEBUG` | Debug level | `''`|
| `FASTIFY_DEBUG` | Debug level for fastify's logger | `''`|
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate env var for enabling fastify logging, since we have our own logging using debug with DEBUG already.

Comment on lines +18 to +19
"test:e2e-flame": "concurrently -k -s all -n \"gateway,playwright\" -c \"magenta,blue\" \"npm run start:dev-flame\" \"wait-on 'tcp:$PORT' && npm run test:e2e\"",
"test:e2e-doctor": "concurrently -k -s all -n \"gateway,playwright\" -c \"magenta,blue\" \"npm run start:dev-doctor\" \"wait-on 'tcp:$PORT' && npm run test:e2e\"",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these no longer seem to be working :(

Comment on lines +55 to +69
"aegir": "41.x",
"clinic": "^13.0.0",
"concurrently": "^8.2.2",
"cross-env": "^7.0.3",
"debug": "4.3.4",
"typescript": "5.x",
"wait-on": "^7.0.1"
"wait-on": "^7.1.0"
},
"dependencies": {
"@helia/unixfs": "1.x",
"blockstore-level": "^1.1.4",
"datastore-level": "^10.1.4",
"dns-over-http-resolver": "2.1.3",
"dns-over-http-resolver": "3.0.0",
"fastify": "4.24.3",
"fastify-metrics": "10.3.2",
"fastify-metrics": "10.3.3",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dependency updates

Comment on lines +21 to +22
"debug:until-death": "./debugging/until-death.sh",
"debug:test-gateways": "./debugging/test-gateways.sh"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

helper scripts to debugging shell scripts.. only tested on macOS M1

reuseExistingServer: process.env.CI == null
reuseExistingServer: process.env.CI == null,
env: {
DEBUG: process.env.DEBUG ?? ' '
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should pass through DEBUG env var to webserver started up by playwright.

Comment on lines +172 to +181
for await (const chunk of await this.heliaFetch.fetch({ address, namespace, relativePath, signal })) {
if (type === undefined) {
type = await parseContentType({ bytes: chunk, path: relativePath })
// this needs to happen first.
reply.raw.writeHead(200, {
'Content-Type': type ?? DEFAULT_MIME_TYPE,
'Cache-Control': 'public, max-age=31536000, immutable'
})
}
reply.raw.write(Buffer.from(chunk))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapped all this in a try-catch to handle bubbling errors on request aborts (and other things that fail, like some webrtc failures I posted as comments in #18)

Comment on lines +183 to +186
} catch (err) {
this.log('Error fetching from Helia:', err)
// TODO: If we failed here and we already wrote the headers, we need to handle that.
// await reply.code(500)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably do something smarter here.

Comment on lines +196 to +202
const opController = new AbortController()
request.raw.on('close', () => {
if (request.raw.aborted) {
this.log('Request aborted by client')
opController.abort()
}
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

listen for request cancellations

Comment on lines +207 to +210
if (address.includes('wikipedia')) {
await reply.code(500).send('Wikipedia is not yet supported. Follow https://github.com/ipfs/helia-http-gateway/issues/35 for more information.')
return
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prevent wikipedia requests

enabled: DEBUG !== '',
msgPrefix: 'helia-http-gateway:fastify',
enabled: FASTIFY_DEBUG !== '',
msgPrefix: 'helia-http-gateway:fastify ',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not having a trailing space caused logs prefixed with helia-http-gateway:fastifyincoming instead of helia-http-gateway:fastify incoming

@SgtPooki
Copy link
Member Author

SgtPooki commented Nov 8, 2023

e2e tests passing. yay! merging this.

@SgtPooki SgtPooki merged commit 1ea5b6d into main Nov 8, 2023
2 checks passed
@SgtPooki SgtPooki deleted the fix/deps branch November 8, 2023 20:51
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 this pull request may close these issues.

1 participant