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

remove ThreadSafeDatastore #120

Merged
merged 1 commit into from
Mar 16, 2019
Merged

remove ThreadSafeDatastore #120

merged 1 commit into from
Mar 16, 2019

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Mar 15, 2019

It's a lie! We:

  1. Assume that our datastores are thread-safe all over the place, not bothering to check for this interface.
  2. Implement this interface for, e.g., the mount datastore that may not be thread-safe (depending on the sub-datastores).

Basically, there's no sane way to to do something like this in go. What we want is:

pub trait ThreadSafe {}

struct MyWrapper<D: Datastore> { ... }

impl<D: Datastore> ThreadSafe for MyWrapper<D> where D: ThreadSafe {}

Actually, we don't even need this because rust has already done all the hard
work with the Sync trait.

....

But we're using go which barely has types.


For completeness, it's actually possible to do this in go:

type threadSafeMixin struct{}
func (threadSafeMixin) ThreadSafe() {}

func NewWrapper(d Datastore) Datastore {
  if _, ok := d.(ThreadSafe) {
    return &struct{myWrapper, threadSafeMixin}{myWrapper{d}, threadSafeMixin{}}
  }
  return &myWrapper{d}
}

Let's not.


Merge first:

It's a lie! We:

1. Assume that our datastores are thread-safe all over the place, not bothering
   to check for this interface.
2. Implement this interface for, e.g., the mount datastore that _may not_ be
   thread-safe (depending on the sub-datastores).

Basically, there's no sane way to to do something like this in go. What we
_want_ is:

```rust
pub trait ThreadSafe {}

struct MyWrapper<D: Datastore> { ... }

impl<D: Datastore> ThreadSafe for MyWrapper<D> where D: ThreadSafe {}
```

Actually, we don't even need this because rust has already done all the hard
work with the `Sync` trait.

....

But we're using go which barely has types.

---

For completeness, it's actually possible to do this in go:

```go
type threadSafeMixin struct{}
func (threadSafeMixin) ThreadSafe() {}

func NewWrapper(d Datastore) Datastore {
  if _, ok := d.(ThreadSafe) {
    return &struct{myWrapper, threadSafeMixin}{myWrapper{d}, threadSafeMixin{}}
  }
  return &myWrapper{d}
}
```

Let's not.
@ghost ghost assigned Stebalien Mar 15, 2019
@ghost ghost added the status/in-progress In progress label Mar 15, 2019
Stebalien added a commit to ipfs/go-ds-redis that referenced this pull request Mar 15, 2019
Stebalien added a commit to ipfs/go-ds-flatfs that referenced this pull request Mar 15, 2019
Stebalien added a commit to ipfs/go-ds-badger that referenced this pull request Mar 15, 2019
Stebalien added a commit to ipfs/go-ds-leveldb that referenced this pull request Mar 15, 2019
@Stebalien Stebalien requested a review from magik6k March 15, 2019 22:56
@Stebalien Stebalien merged commit 6de025e into master Mar 16, 2019
@Stebalien Stebalien deleted the feat/no-thread-safe branch March 16, 2019 04:01
@ghost ghost removed the status/in-progress In progress label Mar 16, 2019
@hsanjuan
Copy link
Contributor

Heads up that this was a breaking API change and was released under a patch version bump (consider doing a minor release bump in that case, as most of us don't expect builds to break after go get -u=patch.

@Stebalien
Copy link
Member Author

You're right, I should have done that. I'm too used to versions meaning nothing at this point...

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.

3 participants