Subscription use channel of pointer of model, may cause thread-unsafe issue?

[sprhawk]

What happened?

The generated subscription, like

func (r *subscriptionResolver) StatusUpdated(ctx context.Context, udid string) (<-chan *model.Status, error) {
...
}

using type of channel of <- chan *model.Status,

when passing pointer of model.Status to multiple goroutine, I think it may cause thread-unsafe problem.

What did you expect?

<-chan *model.Status to <- chan model.Status

func (r *subscriptionResolver) StatusUpdated(ctx context.Context, id string) (<-chan model.Status, error) {
...
}

Minimal graphql.schema and models to reproduce

type Subscription {
    statusUpdated(id: String!) : Status!
}

versions

• gqlgen version? v0.11.3-dev
• go version? go version go1.14.3 linux/amd64
• dep or go modules? go modules

[lwc]

Not sure I follow? Each subscription represents a single connection/goroutine.

If we look at the relevant generated code in the chat example we can see it blocking and waiting for the user supplied subscription resolver to return, then writing the response out: https://github.com/99designs/gqlgen/blob/555db6d20b0157674b5ad05f6ce8856c6502db2e/example/chat/generated.go#L827-L853

[sprhawk]

@lwc I guess you mean Line 842 res, ok := <-resTmp.(<-chan *Message) , so res should be *Message.

There may be multiple clients subscribed to this messageAdded subscription, so I think each endpoint of the clients, will receive a same pointer of *Message, and value passed by pointer cannot assure *Message will not be modified by any means somewhere in some other goroutine / thread.

I think that will be unsafe.

[lwc]

Right - I guess I'm assuming that *Message is created in ec.resolvers.Subscription().MessageAdded(rctx, args["roomName"].(string)) - say via a db lookup or grpc stream etc, but I guess it's possible it could be shared via some stateful service.

I think the impact of this is likely fairly minor and the change would be a BC break, but happy to keep it open and see if it bites anyone.

[sprhawk]

Right - I guess I'm assuming that *Message is created in ec.resolvers.Subscription().MessageAdded(rctx, args["roomName"].(string)) - say via a db lookup or grpc stream etc, but I guess it's possible it could be shared via some stateful service.

I think the impact of this is likely fairly minor and the change would be a BC break, but happy to keep it open and see if it bites anyone.

Yes I did using a *Message from a gRPC message, but due to mutable nature, this will have potential risk.

I think maybe you can set an option in gqlgen.yml to instruct codegen to use *Message or Message specified by developer but leave *Message as current default but marking deprecated or not-recommended

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[frederikhors]

Nope