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.

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

Fix bad format error caused by negative duration #49

Merged
merged 5 commits into from Dec 18, 2020

Conversation

wuYin
Copy link
Contributor

@wuYin wuYin commented Dec 3, 2020

Motivation

Fix issue #48

  • describe

    bad format error occurred if delay or ttr is less than -1s.

  • reproduce

    c, _ := beanstalk.Dial("tcp", "127.0.0.1:11300")
    _, err := c.Put([]byte("abc"), 0, -1*time.Second, 0)
    fmt.Println(err) // put: bad command format
  • cause
    '-' (-1s) is illegal char foruint32, see prot.c#L1048

Modification

dur.String() replaced by dur2sec(time.Duration) string utility function.

@kr
Copy link
Member

kr commented Dec 4, 2020

Hi! Thanks for doing the work to write this patch, but since the server doesn’t support negative durations, I would argue it’s good for this situation to return an error. For example, maybe an application bug has resulted in an unintended negative duration; then it’ll be easier to find that bug if an error shows up.

However, it would certainly help a lot more if a more specific & descriptive error could be returned instead of the general bad format error. It would also be nice to avoid an unnecessary round trip to the server in that case.

How does that sound to you?

@wuYin
Copy link
Contributor Author

wuYin commented Dec 6, 2020

Thanks for review.
Make sense to me, this patch actually doesn't solve the essential problem.
User should find and fix the problem by himself based on the error returned.

@wuYin wuYin closed this Dec 6, 2020
@wuYin wuYin deleted the fix-dur branch December 6, 2020 14:16
@wuYin wuYin restored the fix-dur branch December 11, 2020 03:55
@wuYin
Copy link
Contributor Author

wuYin commented Dec 11, 2020

@kr
Sorry to repoen this PR.
In this bad format case, maybe parameter delaySec uint32 is better than delay time.Duration ?
It's semantic is easier to understand, and with type checking.
The reason for using uint32 is that server read duration actually read uint32, see prot.c#L1073

@wuYin wuYin reopened this Dec 11, 2020
@kr
Copy link
Member

kr commented Dec 18, 2020

Using a uint32 would remove the possibility of negative values, but it would introduce ambiguity about the units (seconds, milliseconds, etc) and it would be a breaking change. I think it’s better to stick with time.Duration, since that’s the standard way to represent a duration in Go.

I think I’d prefer to see a check at the top of method Conn.cmd along these lines:

for _, a := range args {
    if d, _ := a.(time.Duration); d < 0 {
        return req{}, fmt.Errorf("duration must be non-negative, got %v", d)
    }
}

This would still eliminate the need for the internal type dur, like you’ve already done in this change.

@wuYin
Copy link
Contributor Author

wuYin commented Dec 18, 2020

Thanks for review and suggestions, good idea.
Now reserved extensibility for negative duration and without breaking type change, only add type checking for dur before using it

Copy link
Member

@kr kr left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for your patience and perseverance!

@kr kr merged commit 3bed00b into beanstalkd:master Dec 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants