Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Are timeouts on request options specified in milliseconds or nanoseconds? #78

Closed
alexcrichton opened this issue Nov 22, 2023 · 3 comments · Fixed by #85
Closed

Are timeouts on request options specified in milliseconds or nanoseconds? #78

alexcrichton opened this issue Nov 22, 2023 · 3 comments · Fixed by #85

Comments

@alexcrichton
Copy link
Contributor

Currently methods on request-options all end in *-ms which seems to imply that the argument has a unit of milliseconds. The type taken, however, is a duration which specifies that it's in nanoseconds. My guess is that this is an artifact of refactoring that was forgotten, but what's the intention here?

@lukewagner
Copy link
Member

Yes, I think that was mistakenly introduced by a refactoring. The original intention was indeed milliseconds, as suggested by the name. This seems worth fixing in Preview 2, since otherwise it seems very likely to mislead folks.

Given the two obvious choices:

  1. strip the -ms, inheriting the unit from duration (nanoseconds)
  2. keep the -ms, change return type to u64 or a new local type definition

I'd be inclined toward the former, but I think it's also a behavioral change (I'm guessing folks would've implemented milliseconds, given the name), as opposed to the latter which I expect is purely syntactic.

@pchickey
Copy link
Contributor

Lets strip the -ms. I missed that when changing the types to duration.

If we are going to make a change to the RC, can we fix the future-trailers thing as well?

alexcrichton added a commit to alexcrichton/wasi-http that referenced this issue Nov 27, 2023
This is a leftover artifact from recent refactorings but now that it's
using a `duration` type which holds its value in nanoseconds the `-ms`
suffix is no longer required.

Closes WebAssembly#78
@elliottt
Copy link
Collaborator

I put up a draft pr for the fix to wasmtime's implementation: bytecodealliance/wasmtime#7589

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

Successfully merging a pull request may close this issue.

4 participants