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

Accidential non-nil error when batching with WriteAPIBlocking #360

Closed
danielorbach opened this issue Oct 19, 2022 · 1 comment · Fixed by #359
Closed

Accidential non-nil error when batching with WriteAPIBlocking #360

danielorbach opened this issue Oct 19, 2022 · 1 comment · Fixed by #359
Labels
bug Something isn't working
Milestone

Comments

@danielorbach
Copy link
Contributor

Specifications

  • Client Version: v2.10.0
  • InfluxDB Version: v2.4
  • Platform: Docker container

Steps to reproduce

Influx PR draft

  1. Create a new blocking write-API

    c := influxdb2.NewClient(*influxServerURL, *influxAuthToken)
    defer c.Close()
    writeAPI := c.WriteAPIBlocking(*influxOrg, *influxBucket)
  2. Enable batching

    writeAPI.EnableBatching()
  3. Write/Flush

    err := writeAPI.WritePoint(context.Background(), write.NewPointWithMeasurement("m"))
    if err != nil {
        return fmt.Errof("write: %w", err)
    }
    return nil
    err := writeAPI.Flush(context.Background())
    if err != nil {
        return fmt.Errof("flush: %w", err)
    }
    return nil

Expected behavior

We expect calls to Write/Flush to return a non-nil error on successful writes to the database.

Actual behavior

The returned error from Write/Flush is write: <nil> / flush: <nil> instead of a nil error.

Additional info

Looking at the code and commit history, I've found w.service.WriteBatch returns a concrete error type (i.e., *http2.Error).
However, returning a nil pointer (of type *http2.Error) as an interface is not a nil interface (i.e., nil) but a non-nil interface with a valid underlying type and a nil value.
So the test err != nil is always true.

I suggest a quick fix in Pull Request #359 , yet there may be merit in a more comprehensive refactoring to always return type error interface errors instead of the concrete *http2.Error.

Possibly related Pull Requests:

A workaround for those who come across this issue in the meanwhile:

import	http2 "github.com/influxdata/influxdb-client-go/v2/api/http"

// https://github.com/influxdata/influxdb-client-go/pull/359
func workaround(err error) error {
	if e, ok := err.(*http2.Error); ok {
		if e == nil {
			return nil
		}
	}
	return err
}
err := workaround(writer.Flush(ctx))
if err != nil {
	return fmt.Errorf("flush: %w", err)
}
@danielorbach danielorbach added the bug Something isn't working label Oct 19, 2022
danielorbach added a commit to danielorbach/influxdb-client-go that referenced this issue Oct 19, 2022
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
@powersj
Copy link
Contributor

powersj commented Oct 24, 2022

next steps: finish up and land #359

@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
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants