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.

Sign up for GitHub

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

Jump to bottom

fix: catch uncaught exceptions & gc handles request aborts #102

Merged
merged 3 commits into from
Mar 29, 2024
Merged

fix: catch uncaught exceptions & gc handles request aborts #102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ede443f
fix: catch uncaught exceptions & gc handles request aborts
SgtPooki Mar 29, 2024
75d0d01
feat: allow configurable recovery errors
SgtPooki Mar 29, 2024
49dcd6e
chore: remove duplicate gc call
SgtPooki Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/get-custom-helia.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ export async function getCustomHelia (): Promise<Helia> {
}

let blockstore: HeliaInit['blockstore'] | undefined
if (FILE_BLOCKSTORE_PATH != null) {
if (FILE_BLOCKSTORE_PATH != null && FILE_BLOCKSTORE_PATH !== '') {
blockstore = new LevelBlockstore(FILE_BLOCKSTORE_PATH)
}

let datastore: HeliaInit['datastore'] | undefined
if (FILE_DATASTORE_PATH != null) {
if (FILE_DATASTORE_PATH != null && FILE_DATASTORE_PATH !== '') {
datastore = new LevelDatastore(FILE_DATASTORE_PATH)
}

Expand Down
34 changes: 24 additions & 10 deletions src/helia-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,13 +242,7 @@ export class HeliaServer {
}
}

/**
* Fetches a content for a subdomain, which basically queries delegated routing API and then fetches the path from helia.
*/
async fetch ({ request, reply }: RouteHandler): Promise<void> {
const url = this.#getFullUrlFromFastifyRequest(request)
this.log('fetching url "%s" with @helia/verified-fetch', url)

#getRequestAwareSignal (request: FastifyRequest, url = this.#getFullUrlFromFastifyRequest(request), timeout?: number): AbortSignal {
const opController = new AbortController()
setMaxListeners(Infinity, opController.signal)
const cleanupFn = (): void => {
Expand All @@ -272,8 +266,26 @@ export class HeliaServer {
*/
request.raw.on('close', cleanupFn)

if (timeout != null) {
setTimeout(() => {
Copy link
Member

@achingbrain achingbrain Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Timeouts like this use resources and can keep the process running. Better to use AbortSignal.timeout(ms) and combine signals with any-signal.

Copy link
Member Author

Choose a reason for hiding this comment

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

I should have cleared the timeout here, good catch, thanks. any-signal does not prevent duplicate handlers from being added to the same signal, so I prefer not to use it.

I will open an issue to update this to abortSignal.timeout(50) and addEventListener.

Copy link
Member

Choose a reason for hiding this comment

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

any-signal does not prevent duplicate handlers from being added to the same signal, so I prefer not to use it.

Are you passing the same signal to anySignal multiple times? Sounds like a bug if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not, but I know libraries that do :)

Copy link
Member

Choose a reason for hiding this comment

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

If you are not, then it should be okay to use it.

If you know of libraries that do, can you please open issues or better yet, PRs?

this.log.trace('request timed out for url "%s"', url)
opController.abort()
}, timeout)
}
return opController.signal
}

/**
* Fetches a content for a subdomain, which basically queries delegated routing API and then fetches the path from helia.
*/
async fetch ({ request, reply }: RouteHandler): Promise<void> {
const url = this.#getFullUrlFromFastifyRequest(request)
this.log('fetching url "%s" with @helia/verified-fetch', url)

const signal = this.#getRequestAwareSignal(request, url)

await this.isReady
const resp = await this.heliaFetch(url, { signal: opController.signal, redirect: 'manual' })
const resp = await this.heliaFetch(url, { signal, redirect: 'manual' })
await this.#convertVerifiedFetchResponseToFastifyReply(resp, reply)
}

Expand Down Expand Up @@ -319,10 +331,12 @@ export class HeliaServer {
/**
* GC the node
*/
async gc ({ reply }: RouteHandler): Promise<void> {
async gc ({ reply, request }: RouteHandler): Promise<void> {
await this.isReady
this.log('running `gc` on Helia node')
await this.heliaNode?.gc({ signal: AbortSignal.timeout(20000) })
const signal = this.#getRequestAwareSignal(request, undefined, 20000)
await this.heliaNode?.gc({ signal })
await this.heliaNode?.gc()
SgtPooki marked this conversation as resolved.
Show resolved Hide resolved
await reply.code(200).send('OK')
}

Expand Down
25 changes: 24 additions & 1 deletion src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ const stopWebServer = async (): Promise<void> => {
}

let shutdownRequested = false
async function closeGracefully (signal: number): Promise<void> {
async function closeGracefully (signal: number | string): Promise<void> {
log(`Received signal to terminate: ${signal}`)
if (shutdownRequested) {
log('closeGracefully: shutdown already requested, exiting callback.')
Expand All @@ -268,3 +268,26 @@ async function closeGracefully (signal: number): Promise<void> {
// eslint-disable-next-line @typescript-eslint/no-misused-promises
process.once(signal, closeGracefully)
})

/**
* Unless ALLOW_UNHANDLED_ERROR_RECOVERY is set to false, we will attempt to recover from named unhandled errors in the recoverableErrors array.
*/
const allowUnhandledErrorRecovery = process.env.ALLOW_UNHANDLED_ERROR_RECOVERY !== 'false'
const recoverableErrors = ['ERR_STREAM_PREMATURE_CLOSE']
process.on('uncaughtException', (error: any) => {
log.error('Uncaught Exception:', error)
if (allowUnhandledErrorRecovery && recoverableErrors.includes(error.code)) {
log.trace('Ignoring known error')
return
}
void closeGracefully('SIGTERM')
})

process.on('unhandledRejection', (error: any) => {
log.error('Unhandled rejection:', error)
if (allowUnhandledErrorRecovery && recoverableErrors.includes(error.code)) {
log.trace('Ignoring known error')
return
}
void closeGracefully('SIGTERM')
})
Loading