js/grpc: modules.Module migration

[codebien]

Migrated k6/grpc to the modules.Module (aka modules.ModuleV2) interface.

[mstoykov]

LGTM 🎉 . I did leave some non essential nit picks

[olegbespalov]

Thanks! The PR looks good, I left a few suggestions that can improve the readability.

[olegbespalov]

I noticed that there are several places (here and other parts of there code) where we use this check:

state := c.vu.State()
if state == nil {
	return false, errConnectInInitContext
}

How about adding a simple method like modules.IsInitialContext(vu VU) bool I believe that improves the readability of the code, and what is also essential make code more maintainable since we consolidate the logic that identifies that the particular place is dependent during what phase it is executing?

[codebien]

I like the idea in terms of readability, but in this way, we can't reuse the state variable so for most of the methods we are doing 2x on the call for resolving the State that comes from the slow context.

k6/js/initcontext.go

Lines 169 to 171 in 0dda328

	func (m *moduleVUImpl) State() *lib.State {
	return lib.GetState(*m.ctxPtr) // TODO thread it correctly instead
	}

so I'm feeling a bit conflicted here without having some profiling data

[mstoykov]

@olegbespalov please make a separate issue about this.
@codebien in #2228 I in practice stop getting the state from a context, which is a thing that needs to happen for everything at some point either way.

But I do think this should be made ina separate PR and probably across the codebase, so please open an issue @olegbespalov so we can discuss the API for example and whether we want to do this for more stuff

[olegbespalov]

Just to keep the connection, opened an issue #2368

[codebien]

I did some refactor inspired by your suggestions. WDYT?

[olegbespalov]

PR looks good to me, great work! 👍

[mstoykov]

Hm I think we can't remove the goroutine because of #1623 (comment) also ... I really prefer to not change the code all that much in the same PR that already changes a lot of stuff.

If there are some refactoring ideas, please move them to a separate PR, so we can have smaller and easier to review PRs instead of just doing a lot of those in one 🙏

[codebien]

Hm I think we can't remove the goroutine because of #1623 (comment) also

@mstoykov We should be able to remove it as documented in the commit's description c93b8b0

From my understanding, we did it in that way just for getting the TLS error that is covered by grpc.WithReturnConnectionError, I added a test for it in the same commit that fails without using the option. So, I would like to maintain it.

I can move in another PR the stats handler refactor and the reorder. WDYT?

[mstoykov]

This was added in grpc/grpc-go#3430 which was released before the PR that added grpc to k6(and before the comment I linked to). I have no idea if this was just missed or is buggy and just doens't cover all cases 🤷 .

I guess if @rogchap might have some idea, as well as @na--. I have found only one issue grpc/grpc-go#4822, which seems not relevant, but there are others where it seems like TLS errors still seem to not work in some cases. Unfortunately I don't really understand enough to know if those matter or not for our case :(. Your test for example seems to handle what the issue is talking about so 🤷 .

If we can go back to the previous code and have this in a separate PR I think it will be better. We also (AFAIK) don't have that many gRPC users so an introduced bug here will likely stay there for while before we can fix it.

So if it isn't that much work I prefer if this gets split , sorry :(

p.s. thansk for the commit message, and sorry for skip reading it, but unfortunately I am not particularly certain this catches all the cases the previous one did :(

[codebien]

@mstoykov @olegbespalov I restored the codebase without the Connect/dial split. I let only the change for the dedicated method for the parameters' parse.

[mstoykov]

2022/02/03 14:35:21 http: TLS handshake error from 127.0.0.1:50993: EOF
--- FAIL: TestClient (0.00s)
    --- FAIL: TestClient/BadTLS (0.11s)
        client_test.go:675: 
            	Error Trace:	client_test.go:675
            	            				client_test.go:717
            	Error:      	"GoError: context deadline exceeded at reflect.methodValueCall (native)" does not contain "certificate signed by unknown authority"
            	Test:       	TestClient/BadTLS
FAIL
FAIL	go.k6.io/k6/js/modules/k6/grpc	0.397s

seems to be because of the test