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

Add fix for deadline of strings, NaN, Infinity and -Infinity #556

Merged
merged 5 commits into from
May 20, 2019
Merged

Add fix for deadline of strings, NaN, Infinity and -Infinity #556

merged 5 commits into from
May 20, 2019

Conversation

CatEars
Copy link
Contributor

@CatEars CatEars commented May 10, 2019

This will change Infinity to Math.pow(2, 53) - 1 (Number.MAX_SAFE_INTEGER in ES6, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER). Anything that becomes NaN will be discarded (for example setting deadline to a string or NaN), -Infinity will also be discarded.

I don't know if it is best to let bad values for deadlines slip past silently or not...

@CatEars
Copy link
Contributor Author

CatEars commented May 10, 2019

In response to issue #554

Yannic
Yannic previously approved these changes May 14, 2019
Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

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

Sorry for the delaying in reviewing this.

Actually, in the gRPC protocol definition, if Timeout is omitted, it's assumed to be Infinite. So what we should be doing here is that, in L215, if "Infinity" is passed in, just don't set the "grpc-timeout" header. Does it make sense?

@CatEars
Copy link
Contributor Author

CatEars commented May 18, 2019

Sorry for the delaying in reviewing this.

No problem, being an open source maintainer is tough work!

Actually, in the gRPC protocol definition, if Timeout is omitted, it's assumed to be Infinite. So what we should be doing here is that, in L215, if "Infinity" is passed in, just don't set the "grpc-timeout" header. Does it make sense?

I think then that defaulting to 0 is a good idea? I pushed up such a change in 5650699.

Copy link
Collaborator

@stanley-cheung stanley-cheung 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 the contributions and the updates.

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

Successfully merging this pull request may close these issues.

5 participants