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

feat: Add Koa Server Request Timeout #2504

Merged
merged 6 commits into from
May 28, 2024
Merged

feat: Add Koa Server Request Timeout #2504

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9eda6bf
Add Koa Server Request Timeout
Nana-EC May 17, 2024
432089b
fix: Added test and documentation to the server request timeout setting.
ebadiere May 21, 2024
ee2433f
fix: Moved into its own CI job.
ebadiere May 24, 2024
ed825b6
fix: Cleanup
ebadiere May 24, 2024
7fb85ec
fix: Clean up.
ebadiere May 25, 2024
ddbd07c
Merge branch 'main' into 2503-add-server-timeout
ebadiere May 25, 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
6 changes: 5 additions & 1 deletion packages/server/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,11 @@
import app from './server';

async function main() {
await app.listen({ port: process.env.SERVER_PORT || 7546 });
const server = await app.listen({ port: process.env.SERVER_PORT || 7546 });

// set request timeout to ensure sockets are closed after specified time
const requestTimeoutMs = parseInt(process.env.SERVER_REQUEST_TIMEOUT_MS!) || 30000;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Set timeout appropriately.
Maybe 60 secs or more.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

server.setTimeout(requestTimeoutMs);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Add a test to confirm relay sends timeout in this case

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The setTimeout method allows to send a callback function that will be executed in case a timeout is executed:
image

I think we should use it to capture metrics of timeouts.

however, we don't have the registry yet in here...

so I was thinking if we can change the approach and do something like this??

I haven't tested this yet, but basically uses:
https://www.npmjs.com/package/koa-timeout

and move it to file Server.ts

and do it something like this:

// Custom middleware to increment the metric on timeout
app.use(async (ctx, next) => {
  try {
    await next();
  } catch (err) {
    if (err.message === 'Request timeout') {
      // Increment the timeout metric here
      // For example, using a metric library like Prometheus:
      // prometheusClient.counter('request_timeouts_total').inc();
      console.log('Request timeout reached');
    }
    throw err;
  }
});

// Apply the timeout middleware
app.use(timeout(5000)); // Set the timeout value in milliseconds

}

main();
Loading