Skip to content

Fixes #2253 - increase server keep alive timeout#2256

Merged
codyebberson merged 1 commit intomainfrom
cody-server-keep-alive-timeout
Jun 19, 2023
Merged

Fixes #2253 - increase server keep alive timeout#2256
codyebberson merged 1 commit intomainfrom
cody-server-keep-alive-timeout

Conversation

@codyebberson
Copy link
Copy Markdown
Member

See #2253

AWS ALB idle timeout is 60 seconds. Node.js server timeout must be greater than that to avoid 502 Bad Gateway errors. This PR increases the default keep alive timeout to 90 seconds.

@codyebberson codyebberson requested a review from a team as a code owner June 19, 2023 20:50
@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 19, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Updated (UTC)
medplum-storybook ⬜️ Ignored (Inspect) Jun 19, 2023 8:50pm
medplum-www ⬜️ Ignored (Inspect) Jun 19, 2023 8:50pm

return { rows: [{ version: 1000000 }] };
}
if (sql === 'SELECT "User"."id", "User"."content" FROM "User" WHERE "deleted"=$1 LIMIT 1') {
if (sql === 'SELECT "User"."id", "User"."content" FROM "User" WHERE "User"."deleted"=$1 LIMIT 2') {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the query to check "is the database seeded". The existing query was out of date, so the test would unnecessarily re-seed the database. By fixing this line, the test went from 52 seconds to 3 seconds.

@reshmakh reshmakh added the fhir-datastore Related to the FHIR datastore, includes API and FHIR operations label Jun 19, 2023
@reshmakh reshmakh added this to the June 30, 2023 milestone Jun 19, 2023
@coveralls
Copy link
Copy Markdown

Coverage Status

coverage: 94.193%. remained the same when pulling 9f2b2bf on cody-server-keep-alive-timeout into 8bbb31d on main.

@sonarqubecloud
Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

const app = await initApp(express(), config);
app.listen(config.port);
const server = app.listen(config.port);
server.keepAliveTimeout = config.keepAliveTimeout ?? 90000;
Copy link
Copy Markdown
Contributor

@tonylee80 tonylee80 Jun 19, 2023

Choose a reason for hiding this comment

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

Not sure if this applies to us, do we need to have set servers.headersTimeout to be greater than server.keepAliveTimeout as per
nodejs/node#27363 (comment) and
https://shuheikagawa.com/blog/2019/04/25/keep-alive-timeout/?

server.keepAliveTimeout = 61 * 1000;
server.headersTimeout = 65 * 1000; // This should be bigger than `keepAliveTimeout + your server's expected response time`

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Great question. My interpretation of the discussion in that ticket is that the headersTimeout thing was fixed. I'll double check

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

yokomotod commented on Jun 21, 2020
Since nodejs/node#32329 was merged, now I don't need to set headersTimeout bigger than keepAliveTimeout, right ?

@yokomotod if you're asking for v12, v12.19.0 contains the fix

@codyebberson codyebberson merged commit 9fd6db2 into main Jun 19, 2023
@codyebberson codyebberson deleted the cody-server-keep-alive-timeout branch June 19, 2023 23:44
codyebberson added a commit that referenced this pull request Jun 22, 2023
Require prefer: respond-async for super admin tools (#2276)
Suppress console errors on cancelled fetch (#2278)
Fixes #2126 - Moved access helpers to core (#2272)
Only deploy image built with Node.js 18 (#2275)
Remove projectId update from migration (#2274)
Add Non-nullable fields for graphql (#2271)
Add projectId columns to all resource tables (#2270)
Adding additional blog content and updating the number of items blog … (#2258)
Update super-admin-guide.md (#2267)
Dependency upgrades (#2268)
Fixes #2176: Target Directory Options for Bulk CLI commands  (#2265)
Fixes #2261 - Use Data Absent Reason extension for unsupported BCDA r… (#2264)
Update Turbo (#2262)
Validate super admin client assigned IDs (#2263)
Updated discord link (#2260)
Upload Introspection Query (#2214)
Unify the schema loading flows (#2257)
Fixes #2253 - increase server keep alive timeout (#2256)
Fixes #2254 - handle punctuation in ValueSet expand (#2255)
Fixes #2225 - Added AWS Athena guide (#2252)
FhirPathCriteria Docs (#2251)
Add new validator in log-only mode (#2171)
Fixes #2235 - Add docs for maxJsonSize (#2247)
Fixes #2218 - Set vercel install and build commands (#2248)
Updating contributing guides, includes updated images (#2242)
codyebberson added a commit that referenced this pull request Jun 22, 2023
Require prefer: respond-async for super admin tools (#2276)
Suppress console errors on cancelled fetch (#2278)
Fixes #2126 - Moved access helpers to core (#2272)
Only deploy image built with Node.js 18 (#2275)
Remove projectId update from migration (#2274)
Add Non-nullable fields for graphql (#2271)
Add projectId columns to all resource tables (#2270)
Adding additional blog content and updating the number of items blog … (#2258)
Update super-admin-guide.md (#2267)
Dependency upgrades (#2268)
Fixes #2176: Target Directory Options for Bulk CLI commands  (#2265)
Fixes #2261 - Use Data Absent Reason extension for unsupported BCDA r… (#2264)
Update Turbo (#2262)
Validate super admin client assigned IDs (#2263)
Updated discord link (#2260)
Upload Introspection Query (#2214)
Unify the schema loading flows (#2257)
Fixes #2253 - increase server keep alive timeout (#2256)
Fixes #2254 - handle punctuation in ValueSet expand (#2255)
Fixes #2225 - Added AWS Athena guide (#2252)
FhirPathCriteria Docs (#2251)
Add new validator in log-only mode (#2171)
Fixes #2235 - Add docs for maxJsonSize (#2247)
Fixes #2218 - Set vercel install and build commands (#2248)
Updating contributing guides, includes updated images (#2242)
codyebberson added a commit that referenced this pull request Jun 23, 2023
Require prefer: respond-async for super admin tools (#2276)
Suppress console errors on cancelled fetch (#2278)
Fixes #2126 - Moved access helpers to core (#2272)
Only deploy image built with Node.js 18 (#2275)
Remove projectId update from migration (#2274)
Add Non-nullable fields for graphql (#2271)
Add projectId columns to all resource tables (#2270)
Adding additional blog content and updating the number of items blog … (#2258)
Update super-admin-guide.md (#2267)
Dependency upgrades (#2268)
Fixes #2176: Target Directory Options for Bulk CLI commands  (#2265)
Fixes #2261 - Use Data Absent Reason extension for unsupported BCDA r… (#2264)
Update Turbo (#2262)
Validate super admin client assigned IDs (#2263)
Updated discord link (#2260)
Upload Introspection Query (#2214)
Unify the schema loading flows (#2257)
Fixes #2253 - increase server keep alive timeout (#2256)
Fixes #2254 - handle punctuation in ValueSet expand (#2255)
Fixes #2225 - Added AWS Athena guide (#2252)
FhirPathCriteria Docs (#2251)
Add new validator in log-only mode (#2171)
Fixes #2235 - Add docs for maxJsonSize (#2247)
Fixes #2218 - Set vercel install and build commands (#2248)
Updating contributing guides, includes updated images (#2242)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fhir-datastore Related to the FHIR datastore, includes API and FHIR operations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants