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(WriteAPIBlocking): properly return error from flush() #359

Conversation

danielorbach
Copy link
Contributor

@danielorbach danielorbach commented Oct 18, 2022

Commit a9c1e37 introduced a bug, where a successful Flush returns a non-nil error because the internal function (w.service.WriteBatch) returns a concrete pointer and not an error.

Closes #360

This bug is not present without Batching because the return value of WriteBatch() is properly checked before returning from the function writeAPIBlocking.write(). This code path is skipped by delegating the work to writeAPIBlocking.flush() instead.

func (w *writeAPIBlocking) write(ctx context.Context, line string) error {
	if atomic.LoadInt32(&w.batching) > 0 {
		w.mu.Lock()
		defer w.mu.Unlock()
		w.batch = append(w.batch, line)
		if len(w.batch) == int(w.writeOptions.BatchSize()) {
			return w.flush(ctx)
		} else {
			return nil
		}
	}
	err := w.service.WriteBatch(ctx, iwrite.NewBatch(line, w.writeOptions.MaxRetryTime()))
	if err != nil {
		return err
	}
	return nil
}
// flush is unsychronized helper for creating and sending batch
// Must be called from synchronized block
func (w *writeAPIBlocking) flush(ctx context.Context) error {
	if len(w.batch) > 0 {
		body := strings.Join(w.batch, "\n")
		w.batch = w.batch[:0]
		b := iwrite.NewBatch(body, w.writeOptions.MaxRetryTime())
		return w.service.WriteBatch(ctx, b)
	}
	return nil
}

Proposed Changes

check the concrete type for a nil pointer (of type *http2.Error) otherwise return an explicit nil error.

Checklist

  • CHANGELOG.md updated
  • Rebased/mergeable
  • A test has been added if appropriate
  • Tests pass
  • Commit messages are in semantic format
  • Sign CLA (if not already signed)

@telegraf-tiger
Copy link

Thanks so much for the pull request!
🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

@codecov-commenter
Copy link

codecov-commenter commented Oct 18, 2022

Codecov Report

Base: 92.36% // Head: 92.28% // Decreases project coverage by -0.08% ⚠️

Coverage data is based on head (0292c61) compared to base (388612e).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
- Coverage   92.36%   92.28%   -0.09%     
==========================================
  Files          23       23              
  Lines        2227     2228       +1     
==========================================
- Hits         2057     2056       -1     
- Misses        130      131       +1     
- Partials       40       41       +1     
Impacted Files Coverage Δ
api/writeAPIBlocking.go 87.27% <0.00%> (-3.47%) ⬇️
version.go 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@danielorbach
Copy link
Contributor Author

Thanks so much for the pull request! 🤝 ✒️ Just a reminder that the CLA has not yet been signed, and we'll need it before merging. Please sign the CLA when you get a chance, then post a comment here saying !signed-cla

!signed-cla

@vlastahajek vlastahajek changed the title Update writeAPIBlocking.go fix(WriteAPIBlocking): properly return error from flush() Oct 19, 2022
@vlastahajek
Copy link
Contributor

@danielorbach, good point. Thanks for the PR!

Pull Request influxdata#350 (commit a9c1e37)
introduced `Flush()` and `Write()` functions blindly returning the
result of a `func (Service) WriteBatch(...) *http2.Error`.
This causes the returned error to always evaluate as `non-nil`.

Fixes influxdata#360
@danielorbach danielorbach force-pushed the bugfix/WriteAPIBlocking-Flush-always-returns-non-nil-error branch from c652561 to 0292c61 Compare October 19, 2022 16:34
@danielorbach
Copy link
Contributor Author

@vlastahajek Please see the updated description, related Issue (#360) and new commit (0292c61).

Let me know if I should add anything else.

@powersj
Copy link
Contributor

powersj commented Oct 24, 2022

@vlastahajek are you good with the update?

@danielorbach would you be willing to add an entry to the change log as well? That way we can grab this for the release this week.

Thanks!

@vlastahajek vlastahajek merged commit f6f677f into influxdata:master Oct 25, 2022
@danielorbach
Copy link
Contributor Author

@vlastahajek are you good with the update?

@danielorbach would you be willing to add an entry to the change log as well? That way we can grab this for the release this week.

Thanks!

Sorry @powersj I haven't had the time to do it before this PR has been merged.
Would you like me to open another Pull Request for that?

@vlastahajek
Copy link
Contributor

No worries, I've already added the entry.

@vlastahajek vlastahajek added this to the v2.12.0 milestone Oct 27, 2022
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.

Accidential non-nil error when batching with WriteAPIBlocking
4 participants