Fix nil values being added to the reply slice #36
Conversation
s111
force-pushed
the
fix-nil-in-replies
branch
from
01352db
to
1844f16
Compare
TestPerNodeArg is left failing to demonstrate the issue. See #35.
s111
force-pushed
the
fix-nil-in-replies
branch
from
1844f16
to
b5e34b3
Compare
| @@ -120,7 +120,8 @@ func (c *Configuration) readCorrectableStream(ctx context.Context, a *ReadReques | |||
| }() | |||
| } | |||
|
|
|||
| replyChan := make(chan internalState, c.n) | |||
| expected := c.n | |||
There was a problem hiding this comment.
I haven't looked at this PR closely yet.
But this code is not needed? (It does nothing new/change the logic).
Maybe you didn't mean this intentionally since it's a result of the code generation.
And for stream correctables there are no notion of "expected", since the servers can return an arbitrary number of replies.
There was a problem hiding this comment.
The expected variable is technically only needed for the per_node_arg option, but it is perhaps easier to just keep it for both cases to avoid yet another {{if .PerNodeArg}} conditional in the template code. To understand when it is needed you need to look at the generated writePerNode function in calltype_quorumcall_gen.go.
There was a problem hiding this comment.
Yes, my comment was only targeted at the correctables and not in general.
TestPerNodeArg is left failing to demonstrate the issue. See #35.
there is still an outstanding issue related to nil inputs; they don't always panic.
…into fix-nil-in-replies
(the Write test does not work; see TODO comment inline)
|
Merging this into master now. Further changes can be done on master, if there are remaining issues. |
TestPerNodeArg is left failing to demonstrate the issue.
The test should fail while in
masteris succeeds.I'll fix it when we agree whether this is a good solution.
See #35.