Add nil handling on trigger subscriber+publisher #13985
Conversation
|
Do we really have cases in which the message will be nil? @cedric-cordenier |
| @@ -72,6 +73,10 @@ func (p *triggerPublisher) Start(ctx context.Context) error { | |||
| } | |||
|
|
|||
| func (p *triggerPublisher) Receive(_ context.Context, msg *types.MessageBody) { | |||
| if msg == nil { | |||
There was a problem hiding this comment.
@vyzaldysanchez Could we instead fix this category of error in the dispatcher, rather than each receiver? (There are currently 4 receivers and this only fixes 2 cases, and there will be more in the future)
I see two options: 1) passing in a types.MessageBody value (not a pointer) or 2) just checking for nil before sending it on the receiver channel in the dispatcher.
Probably 1 is best, wdyt @bolekk ?
There was a problem hiding this comment.
The fix I'm talking about would be done in this block here:
There was a problem hiding this comment.
target/client.go and target/server.go are indeed also affected
There was a problem hiding this comment.
I'm getting this warning message in the IDE when trying to remove the pointer from the Receive() definitions and calls: 'Receive' passes a lock by the value: type 'types.MessageBody' contains 'protoimpl.MessageState' contains 'sync.Mutex' which is 'sync.Locker'.
Just sharing in case is important to note. It suggests using a pointer instead of just passing the value due to the lock copy that happens.
There was a problem hiding this comment.
Ah I overlooked that it was a proto -- in that case I'd just put the nil check in the caller
|
| @@ -4,7 +4,7 @@ import ( | |||
| "context" | |||
| "errors" | |||
| "fmt" | |||
| sync "sync" | |||
| @@ -114,7 +114,11 @@ func (d *dispatcher) SetReceiver(capabilityId string, donId uint32, rec remotety | |||
| case <-ctx.Done(): | |||
| return | |||
| case msg := <-receiverCh: | |||
| rec.Receive(ctx, msg) | |||
| if msg != nil { | |||
There was a problem hiding this comment.
This can't be nil because ValidateMessage() never returns nil... Do we need this PR at all?
There was a problem hiding this comment.
You are right, I'll close this PR.
Requires Dependencies
Resolves Dependencies