Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable typed nil in versioning ramp #1599

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Enable typed nil in versioning ramp #1599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afefa89
Enable typed nil in versioning ramp
antlai-temporal Aug 17, 2024
2794b90
Cleanup test
antlai-temporal Aug 21, 2024
b1a8ec6
Merge remote-tracking branch 'upstream/master' into fix-typed-nil-ver…
antlai-temporal Aug 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions internal/worker_versioning_rules.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,10 +308,13 @@ func versioningAssignmentRuleToProto(rule *VersioningAssignmentRule) *taskqueuep

switch r := rule.Ramp.(type) {
case *VersioningRampByPercentage:
result.Ramp = &taskqueuepb.BuildIdAssignmentRule_PercentageRamp{
PercentageRamp: &taskqueuepb.RampByPercentage{
RampPercentage: r.Percentage,
},
// Ramp is optional, checking for typed `nil`
if r != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the type switch passes but this is nil, I think this the Ramp value is incorrectly being set. The Ramp value should be nil or a non-nil instance, it should not be a non-nil interface with nil value. Code is going to struggle to use this if rule.Ramp == nil does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was reported by a customer that wrote some innocent looking code that was failing:

r := &VersioningRampByPercentage{
...
}
if (some condition) {
     r = nil
}

I think this should be valid code, otherwise we are really going to confuse people...
But yes, coming from other languages it took me by surprise that nil has type...

Copy link
Member

@cretz cretz Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that wrote some innocent looking code that was failing:

It's not that code exactly. This only happens if you are taking a pointer to a nil variable and assigning it to an interface. See https://go.dev/play/p/F_uAKDduIau.

nil has type...

It does not usually have a type unless it is not being set properly on the interface. What's happening is the interface is non-nil but its implementation is nil. This is a common Go confusion. We don't usually explicitly handle this.

What will happen today if you provide a pointer to a nil *ScheduleWorkflowAction in the Schedule.Action parameter? It will panic, like this code does, and like all of our other places where we accept an interface and people improperly set it to a non-nil value w/ a nil impl.

Your change can work for this one case for that one user that improperly sets the interface to a non-nil-yet-invalid value, but it's inconsistent with all other handling of interfaces we do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the example shared by the customer https://play.golang.com/p/hhUikD1wDvs , yes, you need to do a bit more than that...

Copy link
Member

@cretz cretz Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, this is taking a pointer of a nil variable and assigning it to an interface. This is a known Go situation and in the places we use interfaces, we don't test for nil implementations, just actual nil values. This same issue would happen if a nil struct pointer were provided to something expecting an interface in many parts of our SDK such as schedules or interceptors or something, not just here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we document this somewhere, or this is just standard " good Go API behavior"? The nil struct pointer passed to a function that takes an interface looks like a foot gun...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a standard Go confusion/problem and a standard Go footgun. It is definitely not specific to Temporal and surprising to many users that encounter it the first time. If we want, and @Quinn-With-Two-Ns is ok with, handling this specific interface nilness in this case we can, but in general we never have and most libraries do not and most libraries do not document this Go behavior because it is known (even if it is a footfun).

Copy link

@Drahflow Drahflow Aug 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cretz Said customer from above here.

It's not that code exactly.

It is exactly that code which lead to the nil access prompting my original report. The ramp := &VersioningRampByPercentage creates the variable with *VersioningRampByPercentage type (which becomes a typed nil once passed within VersioningAssignmentRule if it was nil).

In the original report ( https://temporalio.slack.com/archives/CTRCR8RBP/p1723805024798129?thread_ts=1714989745.540629&cid=CTRCR8RBP ) I also mentioned "And I cannot declare ramp of type internal.VersioningRamp to avoid that, because it's not re-exported by client afaics."

result.Ramp = &taskqueuepb.BuildIdAssignmentRule_PercentageRamp{
PercentageRamp: &taskqueuepb.RampByPercentage{
RampPercentage: r.Percentage,
},
}
}
}

Expand Down Expand Up @@ -419,8 +422,10 @@ func (r *VersioningAssignmentRule) validateRule() error {
}
switch ramp := r.Ramp.(type) {
case *VersioningRampByPercentage:
if err := ramp.validateRamp(); err != nil {
return err
if ramp != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can me move the nil check out of the switch so when we add more Ramp types we don't need to duplicate this check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ramp is an interface and to know that a non-nil interface contains a nil pointer I need to type cast. Can't think of a way to do that without repeating the type cast for each concrete type, kind of defeating the purpose of lifting the check...

if err := ramp.validateRamp(); err != nil {
return err
}
}
// Ramp is optional, defaults to "nothing to validate"
}
Expand Down
20 changes: 20 additions & 0 deletions internal/worker_versioning_rules_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,23 @@ func Test_VersioningIntent(t *testing.T) {
})
}
}

func Test_WorkerVersioningRules_typed_nil(t *testing.T) {
ramp := &VersioningRampByPercentage{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ramp := &VersioningRampByPercentage{
var ramp *VersioningRampByPercentage

I think this should be equivalent and show what your trying to test a bit clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, done, I was doing cut/paste from another example...

Percentage: 45.0,
}
ramp = nil

u := UpdateWorkerVersioningRulesOptions{
TaskQueue: "myQueue",
Operation: &VersioningOperationInsertAssignmentRule{
RuleIndex: 0,
Rule: VersioningAssignmentRule{
TargetBuildID: "2.0",
Ramp: ramp,
},
},
}
_, err := u.validateAndConvertToProto("my_namespace")
assert.NoError(t, err)
}
Loading