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

Set max session idle time #642

Merged
merged 15 commits into from
Jun 14, 2022
Merged

Set max session idle time #642

merged 15 commits into from
Jun 14, 2022

Conversation

henryfauna
Copy link
Contributor

@henryfauna henryfauna commented Jun 8, 2022

Notes

Jira Ticket

Previously "Infinity" was a possible value for http2SessionIdleTime. This change enforces a maximum value of 5000 milliseconds.

How to test

yarn test

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
test/client.test.js Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
@cleve-fauna
Copy link
Contributor

Noting that @henryfauna mentioned @faunaee called out we should determine how this impacts streaming, as it could change our implementation. (good call out @faunaee !)

@cleve-fauna
Copy link
Contributor

@henryfauna please prepare this as a new driver version following the guide here: https://faunadb.atlassian.net/wiki/spaces/DX/pages/3002335237/Drivers#JS-driver-release

@cleve-fauna cleve-fauna requested a review from faunaee June 13, 2022 22:15
cleve-fauna
cleve-fauna previously approved these changes Jun 13, 2022
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

I've made a few wording suggestions, wondered about the env var precedence, and asked about a change to the stream testing.

README.md Outdated Show resolved Hide resolved
src/Client.js Outdated Show resolved Hide resolved
test/client.test.js Show resolved Hide resolved

const { ts: newTimestamp } = await client.query(q.Update(doc.ref, {}))
expect(newTimestamp).toBeGreaterThan(oldTimestamp)
await util.delay(idleTime + 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be worth checking that the stream is still open beyond the maximum value of http2SessionIdleTime? As written, this test doesn't really demonstrate that a stream is held open indefinitely, just that it is held open for 2002 ms (which is well within 5000ms).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test should be updated now

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

I noticed that the code determine the value to use for http2SessionIdleTime seems more complex than it needs to be.

src/Client.js Outdated
for (const rawValue of values) {
const parsedValue = parseInt(rawValue, 10)
const isNegative = parsedValue < 0
const isInfinity = rawValue === 'Infinity'
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be easier to do this check first?

The overall check could be simpler, too:

// Handle Infinity, and NaN (for any other string)
const parsedValue = rawValue === 'Infinity'
  ? Number.MAX_SAFE_INTEGER
  : parseInt(rawValue, 10) || defaultIdleTime

// Handle upper bound
if (parsedValue > maxIdleTime) parsedValue = maxIdleTime

// Handle lower bound
if (parsedValue < 0) parsedValue = defaultIdleTime

return parsedValue

faunaee
faunaee previously approved these changes Jun 14, 2022
Copy link
Contributor

@faunaee faunaee left a comment

Choose a reason for hiding this comment

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

Thanks for applying my suggestions!

LGTM 👍

Note: I've updated the PR for DOCS-2096 to note that http2SessionIdleTime does not affect stream connections.

cleve-fauna
cleve-fauna previously approved these changes Jun 14, 2022
@henryfauna henryfauna dismissed stale reviews from cleve-fauna and faunaee via 4742d8f June 14, 2022 18:22
@henryfauna henryfauna merged commit 4d8537b into v4 Jun 14, 2022
@henryfauna henryfauna deleted the FE-2238 branch June 14, 2022 19:20
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.

None yet

3 participants