gqlgen
gqlgen copied to clipboard
Subscription use channel of pointer of model, may cause thread-unsafe issue?
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
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
@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.
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.
Right - I guess I'm assuming that
*Message
is created inec.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
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.
Nope