fix: handle NaN and Infinity float values in gRPC #4631

ariasmn · 2025-03-15T11:31:32Z

Fixes (?): #3990

The problem was in this specific line:

k6/internal/js/modules/k6/grpc/client.go

Line 363 in a74d803

b, err := req.ToObject(c.vu.Runtime()).MarshalJSON()

This MarshalJSON implemented in Sobek uses a copy of JSON.stringify() under the hood, and per JSON RFC, neither Infinity or NaN as values are accepted as valid numbers.

However, since we rely on ProtoJSON, which does support special strings "Infinity" and "NaN", I guess that the idea would be to "normalize" them as a string format before marshaling, which is what this PR implements.

ariasmn · 2025-03-15T11:54:33Z

Couple of things:

Having a hard time trying to understand how to build the tests (I'm guessing that it should be a test case in here). I just checked that current tests pass and added one for wrapped double value, but I guess I'd have to add another one for a normal float proto.
I still don't think that this approach is good enough... I feel like maybe the best bet would be to create the proto message using protoreflect and then marshaling normally, but I haven't tried if that would break wrappers and reflection implementations (and also is a bit long in terms of code, atleast the way I did it at my company).

mstoykov

Thank you for the PR and sorry for the slow reply 🙇

I have left some comments around the code.

On the whole idea I am still not particularly certain this is ... what should be done, if anything.

Trying to figure out how this works in other cases I found protocolbuffers/protobuf-javascript#49 which seems to suggest that this isn't really supported. But maybe I am getting something wrong.

From the test it seems that this just works on the way back which seems awfully convient.

internal/js/modules/k6/grpc/client.go

ariasmn · 2025-04-11T18:54:09Z

Hey @mstoykov, thanks a lot for the review!

I’ve made some changes based on your comments. There’s one part I didn't change yet because I’m not fully sure I understand it correctly, but we can keep discussing it in the open conversation.

Regarding the issue you mentioned: it looks like an older open issue. From what I saw, the code has since changed and now properly asserts both Infinity and NaN:

https://github.com/protocolbuffers/protobuf-javascript/blob/0768cc9f18bec192c393d7c2962ee8ab5ad1931b/binary/encoder.js#L411-L418

I also tested it locally using the dummy.proto from the issue, for both client and server (wire format). Additionally, the ProtoJSON documentation confirms that both values are officially supported.

Just a heads-up: I might be a bit slower next week if any further changes are needed — apologies in advance.

Thanks again for the great feedback. 🙏

EDIT: Tests are failing, don't know if related to my changes but the client_test.go tests are passing correctly locally 🤔

ariasmn · 2025-06-20T17:48:40Z

Hi there,

sorry for the delay on this! It's been a while since I could get back to this PR (I haven't had much time recently).

I've made the requested corrections. Please take a look and let me know if everything looks good (I feel like there should be a better way of handling the copy for sure) or if there's anything else that needs to be addressed.

Thank you! 🙇

joanlopez · 2025-07-22T11:56:34Z

Hi @ariasmn,

Sorry for the late review, but finally I've been able to properly test it.

I think it's close to be ready to be merged, but there's one extra thing that we should fix, which is that if normalizeNumberStrings receives a cyclic reference to an object (see below), it will end up in an infinite loop of recursive calls that eventually will lead to a panic.

For instance:

client.connect("GRPCBIN_ADDR");

let obj1 = {};
let obj2 = {};
obj1.a = obj2;
obj2.b = obj1;  // Now both objects reference each other

let respNaN = client.invoke("grpc.wrappers.testing.Service/TestDouble", obj1); // <-- this causes the panic

Can you take a look at this and try to fix it, please? Thanks!

…ring-float' into fix/grpc-special-string-float

Conflicts: internal/js/modules/k6/grpc/client_test.go

ariasmn · 2025-07-27T10:41:05Z

Hi @joanlopez,

Thank you for catching that one! I implemented a fix by following what's done in the stringify method in Sobek, and added a test case as well:

k6/vendor/github.com/grafana/sobek/builtin_json.go

Lines 366 to 372 in 602467d

    
           case *Object: 
        
           	for _, object := range ctx.stack { 
        
           		if value1.SameAs(object) { 
        
           			ctx.r.typeErrorResult(true, "Converting circular structure to JSON") 
        
           		} 
        
           	} 
        
           	ctx.stack = append(ctx.stack, value1)

Let me know if my choice of wording for variables/comments is odd, I can fix them no problem.

Thanks!

joanlopez · 2025-07-28T05:11:22Z

internal/js/modules/k6/grpc/client.go

+	}
+
+	// Handle special float values.
+	if f, ok := v.Export().(float64); ok {


As @mstoykov suggested before, could we use ExportType() here instead of Export(), as the latter is a much more expensive operation.

In case it's a float64, I guess you'll need to call v.Export() anyway, but in all the other cases, it won't be called that early.

Or maybe even something like:

Suggested change

if f, ok := v.Export().(float64); ok {

if v.ExportType().Kind() == reflect.Float64 {

f := v.ToFloat()

switch {

case math.IsNaN(f):

return runtime.ToValue("NaN"), nil

case math.IsInf(f, 1):

return runtime.ToValue("Infinity"), nil

case math.IsInf(f, -1):

return runtime.ToValue("-Infinity"), nil

}

return v, nil

}

So, you don't need Export() at all.
Wdyt? @mstoykov

This seems better.

For the record - the idea is that if you are using Export it will do a lot of work in some of the cases. If the end type is flaot64 - that probably is fine. But it also might be a big object that you just fully transformed from JS to go type. And then you just throw it away because it isn't a float64, just to repeat the operation each time.

Sorry about that, I resolved the previous conversation thinking I had already implemented the change, totally my bad.
Thanks for the explanation and sorry for the inconvenience!

Let me know if there's anything else I can do on my end.

Sure, no problem @ariasmn, that's why we have a review process, we're all humans and sometimes it happens!

Looks good now! 👌🏻 Thanks! 🙌🏻

mstoykov

LGTM! Thanks for the changes and going through figuring it out @ariasmn 🙇

ariasmn added 3 commits March 14, 2025 23:17

fix: first iter to parse NaN/Infinity as string

3291c97

fix: better solution, without test case

60244e8

fix: lint

fdd70b2

ariasmn force-pushed the fix/grpc-special-string-float branch from 2fa5e78 to fdd70b2 Compare March 15, 2025 11:46

ariasmn mentioned this pull request Mar 15, 2025

NaN float values fail in gRPC tests and in console.log #3990

Closed

ariasmn added 3 commits March 15, 2025 12:59

fix: remove dummy.proto

8983aa7

fix: forgotten stash

32edbac

fix: add tests and handle wrappers values

90921fd

mimani68 approved these changes Mar 16, 2025

View reviewed changes

ariasmn marked this pull request as ready for review March 20, 2025 15:26

ariasmn requested a review from a team as a code owner March 20, 2025 15:26

ariasmn requested review from mstoykov and joanlopez and removed request for a team March 20, 2025 15:26

joanlopez requested a review from oleiade March 20, 2025 15:50

mstoykov reviewed Apr 11, 2025

View reviewed changes

internal/js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

internal/js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

internal/js/modules/k6/grpc/client.go Outdated Show resolved Hide resolved

ariasmn added 2 commits April 11, 2025 20:39

fix: PR comments

b77f357

Merge branch 'master' into fix/grpc-special-string-float

a598ec9

ariasmn added 2 commits April 11, 2025 20:56

fix: lint

84bcdee

Merge branch 'master' into fix/grpc-special-string-float

e34d17b

ariasmn requested a review from mstoykov May 2, 2025 18:47

ariasmn added 2 commits June 20, 2025 18:51

Merge branch 'master' into fix/grpc-special-string-float

5cdb02f

Fix BC raised on PR comment by creating a deep copy

adbc0f9

ariasmn added 4 commits July 17, 2025 16:26

fix: first iter to parse NaN/Infinity as string

a3387e4

fix: better solution, without test case

8a43452

fix: lint

cfc27a0

fix: remove dummy.proto

5f3c684

ariasmn added 5 commits July 17, 2025 16:26

fix: forgotten stash

4dbeb76

fix: add tests and handle wrappers values

81a4527

fix: PR comments

8dbe476

fix: lint

d130a34

Fix BC raised on PR comment by creating a deep copy

0a673e0

mstoykov force-pushed the fix/grpc-special-string-float branch from adbc0f9 to 0a673e0 Compare July 17, 2025 13:26

joanlopez added this to the v1.2.0 milestone Jul 22, 2025

joanlopez added awaiting user waiting for user to respond area: grpc labels Jul 22, 2025

joanlopez self-assigned this Jul 22, 2025

ariasmn added 3 commits July 26, 2025 12:16

Merge remote-tracking branch 'refs/remotes/origin/fix/grpc-special-st…

4734907

…ring-float' into fix/grpc-special-string-float

fix: cyclic reference to object

b15efdf

Merge branch 'master' into fix/grpc-special-string-float

a15278f

Conflicts: internal/js/modules/k6/grpc/client_test.go

joanlopez reviewed Jul 28, 2025

View reviewed changes

joanlopez requested a review from a team July 28, 2025 05:19

fix: prefer ExportType over Export for better performance

2971ae3

joanlopez approved these changes Jul 29, 2025

View reviewed changes

mstoykov approved these changes Jul 31, 2025

View reviewed changes

mstoykov merged commit 5bc3231 into grafana:master Jul 31, 2025
39 checks passed

mstoykov added documentation-needed A PR which will need a separate PR for documentation and removed awaiting user waiting for user to respond labels Aug 4, 2025

joanlopez mentioned this pull request Aug 8, 2025

gRPC: Document the special handling of non-numerical float values grafana/k6-docs#2020

Merged

8 tasks

BrewTestBot mentioned this pull request Aug 13, 2025

k6 1.2.1 Homebrew/homebrew-core#233339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: handle NaN and Infinity float values in gRPC #4631

fix: handle NaN and Infinity float values in gRPC #4631

ariasmn commented Mar 15, 2025 •

edited

Loading

Uh oh!

ariasmn commented Mar 15, 2025 •

edited

Loading

Uh oh!

mstoykov left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariasmn commented Apr 11, 2025 •

edited

Loading

Uh oh!

ariasmn commented Jun 20, 2025

Uh oh!

joanlopez commented Jul 22, 2025 •

edited

Loading

Uh oh!

ariasmn commented Jul 27, 2025

Uh oh!

joanlopez Jul 28, 2025

Uh oh!

joanlopez Jul 28, 2025

Uh oh!

mstoykov Jul 28, 2025

Uh oh!

ariasmn Jul 28, 2025

Uh oh!

joanlopez Jul 29, 2025

Uh oh!

mstoykov left a comment

Uh oh!

Uh oh!

Uh oh!

-	if f, ok := v.Export().(float64); ok {
+	if v.ExportType().Kind() == reflect.Float64 {
+		f := v.ToFloat()
+		switch {
+		case math.IsNaN(f):
+			return runtime.ToValue("NaN"), nil
+		case math.IsInf(f, 1):
+			return runtime.ToValue("Infinity"), nil
+		case math.IsInf(f, -1):
+			return runtime.ToValue("-Infinity"), nil
+		}
+		return v, nil
+	}

fix: handle NaN and Infinity float values in gRPC #4631

fix: handle NaN and Infinity float values in gRPC #4631

Conversation

ariasmn commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariasmn commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstoykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ariasmn commented Apr 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariasmn commented Jun 20, 2025

Uh oh!

joanlopez commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ariasmn commented Jul 27, 2025

Uh oh!

joanlopez Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

joanlopez Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

mstoykov Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

ariasmn Jul 28, 2025

Choose a reason for hiding this comment

Uh oh!

joanlopez Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

mstoykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ariasmn commented Mar 15, 2025 •

edited

Loading

ariasmn commented Mar 15, 2025 •

edited

Loading

ariasmn commented Apr 11, 2025 •

edited

Loading

joanlopez commented Jul 22, 2025 •

edited

Loading