Calling non-Close() methods on closed or written batches panic #71
Conversation
cleveldb_batch.go
Outdated
| return err | ||
| } | ||
| return nil | ||
| return b.Close() |
There was a problem hiding this comment.
do we even need a separate Close method? if we execute Close in Write/WriteSync
There was a problem hiding this comment.
Yeah, because the caller may e.g. return an error while they are building the batch, in which case they need to close it without calling Write.
There was a problem hiding this comment.
I can easily envision cases where Close will be called twice... maybe it's okay, but I'd prefer to remove Close calls from Write/WriteSync. You're mixing responsibilities
There was a problem hiding this comment.
Calling Close() twice is fine, it is idempotent. It is a normal Go pattern to use defer c.Close() for error handling and then also do an explicit c.Close() as early as possible to free up resources before the function exits.
If we don't call Close() in Write(), then what should happen when a caller calls Write() twice?
There was a problem hiding this comment.
It is a normal Go pattern to use defer c.Close() for error handling and then also do an explicit c.Close() as early as possible to free up resources before the function exits.
Didn't know about such.
If we don't call Close() in Write(), then what should happen when a caller calls Write() twice?
you can't prevent people from shooting themselves into the leg. I'd assume correct usage in most cases.
New -> Set/Delete -> Write(Sync) -> Close (via defer)
There was a problem hiding this comment.
you can't prevent people from shooting themselves into the leg. I'd assume correct usage in most cases.
Of course, we just have to specify what should happen, as unspecified behavior is the root of all evil. Either Write() also calls Close(), in which case a second Write() call errors, otherwise the second Write()call should write the same batch again.
I think erroring is safer, and easier to implement across all backends. The typical and recommended usage would still be the same flow that you outline.
There was a problem hiding this comment.
I think erroring is safer, and easier to implement across all backends.
Agree.
There was a problem hiding this comment.
Ok, I think this should be good to go then.
erikgrinaker
force-pushed
the
erik/batch-errors
branch
from
f92256d
to
1b5ff54
Compare
erikgrinaker
changed the title
Bumps [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action) from 2.8.0 to 2.9.0. - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](docker/setup-buildx-action@v2.8.0...v2.9.0) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
See discussion in #66 (review).