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(middleware): Introduce Timeout Middleware #2615

Merged
merged 22 commits into from
May 22, 2024
Merged

Conversation

watany-dev
Copy link
Contributor

I used to use it for personal use, but I see here that there seems to be a demand for it, so I share it with you.
https://github.com/orgs/honojs/discussions/1765

Many cloud environments implement infrastructure-level timeouts, but it is useful if you want to set per-route timeouts on the application side.


Timeout Middleware

This middleware enables you to easily manage request timeouts in your application. It allows you to set a maximum duration for requests and define custom error responses if the specified timeout is exceeded.

Import

import { Hono } from 'hono';
import { timeout } from 'hono/timeout';

Usage

Here's how to use the Timeout Middleware with default and custom settings:

Default Settings:

const app = new Hono();

// Applying a 5-second timeout
app.use('/api', timeout(5000));

// Handling a route
app.get('/api/data', async (c) => {
  // Your route handler logic
  return c.json({ data: 'Your data here' });
});

Custom Timeout Settings:

app.use('/api/long-process', timeout('1m', {
  message: 'Request timeout. Please try again later.',
  code: 408,
}));

app.get('/api/long-process', async (c) => {
  // Simulate a long process
  await new Promise(resolve => setTimeout(resolve, 61000));
  return c.json({ data: 'This usually takes longer' });
});

Understanding Timeouts

The duration for the timeout can be specified in milliseconds or as a string in a human-readable format, such as '1m' for one minute or '10s' for ten seconds. The parseDuration function translates these string values into milliseconds.

Middleware Conflicts

Be cautious about middleware order, especially when using error-handling or other timing-related middleware, as it might affect the behavior of this timeout middleware.


Author should do the followings, if applicable

  • Add tests
  • Run tests
  • bun denoify to generate files for Deno


export const timeout = (
duration: number | string,
options: TimeoutOptions = {}
Copy link
Member

Choose a reason for hiding this comment

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

How about taking exception as the second arg:

export const timeout = (
  duration: number | string,
  exception?: HTTPException | ((c: Context) => HTTPException)
)

How to use it (callback pattern):

app.get(
  '/',
  timeout(1000, (c) => {
    return new HTTPException(408, {
      message: `Timeout occurred at ${c.req.path}`,
    })
  }),
  async (c) => {
    // ...
  }
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. I fixed it, can you see if the type definitions and other perceptions you think match?

@yusukebe
Copy link
Member

yusukebe commented May 6, 2024

@watany-dev

Thank you for the PR. I like this! But we have to discuss some points. One is that I've left a comment.

Is parseDuration necessary?

Honestly, I don't think parseDuration is not necessary. It's OK to get only a number of ms in timeout():

export const timeout = (
  duration: number,
  //
)

We've discussed the Body Limit Middleware in #2103 about a similar issue. In the Body Limit Middleware case, we decided not to accept introducing "Unit" a2c35dd. I also think we don't need the parseDuration method for the same reason. If we want to add 11s, we can simply write 1000 * 11.

What do you think?

@yusukebe
Copy link
Member

yusukebe commented May 6, 2024

Hey @usualoma, I want to know your thoughts on this PR. I think it's a good idea!

@usualoma
Copy link
Member

usualoma commented May 7, 2024

@watany-dev Thank you!
I agree with merging this PR, as it is a useful feature.

parseDuration

It is close to the alternative one of my following comments on BodyLimit, which I think is one possible way of doing it.
#2103 (comment)

However, well, It seems to me that only number is sufficient, given the following points.

  • That hono's other middleware does not convert units.
  • JavaScript (or TypeScript) users are used to setTimeout() and setInterval(), and are used to milliseconds when specifying duration

If this PR continues to use parseDuration

The following strings should be made an error.

'500milliseconds'

const errorCode = options.code ?? DEFAULT_ERROR_CODE
const ms = typeof duration === 'string' ? parseDuration(duration) : duration

return async (context, next) => {
Copy link
Member

@usualoma usualoma May 7, 2024

Choose a reason for hiding this comment

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

To make it easier for users to debug, it would be better to return a named function like other middleware.

return async function bodyLimit(c, next) {

Copy link
Contributor Author

@watany-dev watany-dev May 7, 2024

Choose a reason for hiding this comment

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

thx! I fixed 758a334.

@watany-dev
Copy link
Contributor Author

watany-dev commented May 7, 2024

@yusukebe @usualoma
Thank you for your comment. Actually, I also think that it would be sufficient to just use milliseconds for the duration. I adopted this approach because there was a user of another middleware who used it. I will keep it simple by only allowing the duration in milliseconds as a number.

@watany-dev
Copy link
Contributor Author

watany-dev commented May 7, 2024

Thank you. I think I have mostly rewritten it. Here is the revised spec document.


Timeout Middleware

This middleware enables you to easily manage request timeouts in your application. It allows you to set a maximum duration for requests and optionally define custom error responses if the specified timeout is exceeded.

Import

import { Hono } from 'hono';
import { timeout } from 'hono/timeout';

Usage

Here's how to use the Timeout Middleware with both default and custom settings:

Default Settings:

const app = new Hono();

// Applying a 5-second timeout
app.use('/api', timeout(5000));

// Handling a route
app.get('/api/data', async (c) => {
  // Your route handler logic
  return c.json({ data: 'Your data here' });
});

Custom Timeout Settings:

import { HTTPException } from 'hono/http-exception';

// Custom exception factory function
const customTimeoutException = (context) => new HTTPException(408, {
  message: `Request timeout after waiting ${context.req.headers.get('Duration')} seconds. Please try again later.`,
});

// for Static Exception Message
// const customTimeoutException = new HTTPException(408, {
//   message: 'Operation timed out. Please try again later.'
// });

// Applying a 1-minute timeout with a custom exception
app.use('/api/long-process', timeout(60000, customTimeoutException));

app.get('/api/long-process', async (c) => {
  // Simulate a long process
  await new Promise(resolve => setTimeout(resolve, 61000));
  return c.json({ data: 'This usually takes longer' });
});

Note

  • The duration for the timeout can be specified in milliseconds. The middleware will automatically reject the promise and potentially throw an error if the specified duration is exceeded.

  • The timeout middleware cannot be used with stream Thus, use stream.close and setTimeout together.

app.get('/sse', async (c) => {
  let id = 0
  let running = true;
  let timer: number | undefined

  return streamSSE(c, async (stream) => {
    timer = setTimeout(() => {
      console.log("Stream timeout reached, closing stream");
      stream.close();  
    }, 3000) as unknown as number

    stream.onAbort(async () => {
      console.log("Client closed connection");
      running = false;
      clearTimeout(timer);
    });

    while (running) {
      const message = `It is ${new Date().toISOString()}`
      await stream.writeSSE({
        data: message,
        event: 'time-update',
        id: String(id++),
      })
      await stream.sleep(1000)
    }
  })
})

Middleware Conflicts

Be cautious about the order of middleware, especially when using error-handling or other timing-related middleware, as it might affect the behavior of this timeout middleware.

@yusukebe
Copy link
Member

yusukebe commented May 7, 2024

@watany-dev

Thank you. I think I have mostly rewritten it. Here is the revised spec document.

Awesome! I like it.

But I have a problem with the current code. I tried the following example on Cloudflare Workers with Wrangler and Bun:

import { Hono } from '../../src'
import { HTTPException } from '../../src/http-exception'
import { timeout } from '../../src/middleware/timeout'

const app = new Hono()

app.get(
  '/',
  timeout(1000, (c) => {
    return new HTTPException(408, {
      message: `Timeout occurred at ${c.req.path}`,
    })
  }),
  async (c) => {
    await new Promise((resolve) => setTimeout(resolve, 3000))
  }
)

export default app

It behaves weirdly. See the screencast:

Area.mp4

It works correctly in the first request. But from the second request, it will not stop until 3 seconds have passed, and it is accessed 3 times. This behavior happens to both Wrangler and Bun.

Can you see it?

@watany-dev
Copy link
Contributor Author

I will check. As I was writing the Stream example above, I thought that maybe the clearTimeout is not working correctly.

@watany-dev
Copy link
Contributor Author

Note.
I did not reproduce it with bun 1.1.6, and Cloudflare workers. I don't have time today, I will try again after tomorrow.

@yusukebe
Copy link
Member

yusukebe commented May 7, 2024

Thanks @watany-dev

By the way. If I don't set the second arg like the following, it works well.

const app = new Hono()

app.get('/', timeout(1000), async (c) => {
  await new Promise((resolve) => setTimeout(resolve, 3000))
})

export default app

@usualoma
Copy link
Member

usualoma commented May 7, 2024

I believe this is Google Chrome's behaviour for 408

https://groups.google.com/a/chromium.org/g/chromium-dev/c/urswDsm6Pe0/m/JBObkCT0AAAJ

According to the RFC, requests can be repeated in status 408, so I think Google Chrome is resending requests accordingly.

If the client has an outstanding request in transit, the client MAY repeat that request on a new connection.
https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.7

@yusukebe
Copy link
Member

yusukebe commented May 7, 2024

@usualoma

I believe this is Google Chrome's behaviour for 408

You are right! I changed the status code 408 to 504, and it works as expected!

@watany-dev

Sorry for bothering you (If you paid for Cloudflare only for this purpose, super sorry!).

@watany-dev
Copy link
Contributor Author

@yusukebe
Thank you for your feedback. I've also corrected the comments section. I'm looking forward to experimenting with Cloudflare Workers :)

@watany-dev watany-dev requested a review from yusukebe May 8, 2024 04:02
package.json Show resolved Hide resolved
@yusukebe yusukebe added v4.4 and removed v4.3 labels May 10, 2024
Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

Hey @watany-dev !

Looks good! Let's goo with this. We'll release it in the next minor versionv4.4. Let's leave this open until I add the next branch later and merge this into it. Thanks!

@watany-dev
Copy link
Contributor Author

@yusukebe
I've added a JS Doc.

@fzn0x
Copy link
Contributor

fzn0x commented May 16, 2024

I'm thinking we should improve the PR template to tell every PR that adding a new feature should add TSDoc/JSDoc (if it's necessary) @yusukebe This way we can slowly migrate codebase to use TSDoc/JSDoc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants