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

RevocationDB fixes #866

Merged
merged 4 commits into from Dec 14, 2018
Merged

Conversation

bryanchriswhite
Copy link
Contributor

No description provided.

@cla-bot cla-bot bot added the cla-signed label Dec 14, 2018
@bryanchriswhite bryanchriswhite added the Request Code Review Code review requested label Dec 14, 2018
@@ -82,21 +82,23 @@ type ServerConfig struct {
type ServerOptions struct {
Config ServerConfig
Ident *FullIdentity
RevDB *peertls.RevocationDB
Copy link
Member

@egonelbre egonelbre Dec 14, 2018

Choose a reason for hiding this comment

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

It looks very weird to close "Options".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have an alternate name or flow in mind?

Copy link
Member

Choose a reason for hiding this comment

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

not at the moment

@bryanchriswhite bryanchriswhite changed the title close revDB RevocationDB fixes Dec 14, 2018
@@ -294,11 +296,14 @@ func (ic IdentityConfig) Run(ctx context.Context, interceptor grpc.UnaryServerIn
if err != nil {
return err
}
defer func() { _ = opts.RevDB.Close() }()
Copy link
Member

Choose a reason for hiding this comment

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

Check the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What should we do with an error in a defer? Or should it not be in a defer?

Copy link
Member

Choose a reason for hiding this comment

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

There's no really nice way of handling them at the moment.

One way would be to use something like:

defer func() { err = utils.CombineErrors(err, opts.RevDB.Close()) }

The other would be to manually close everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand how your defer example would do anything useful and I'm pretty sure we want to use a defer for this

Copy link
Member

Choose a reason for hiding this comment

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

Take a look at https://play.golang.org/p/0qh7EuHW7Ut. It relies on the behavior that you can change the returned err variable.

@bryanchriswhite bryanchriswhite merged commit d8db7c3 into storj:master Dec 14, 2018
@bryanchriswhite bryanchriswhite removed the Request Code Review Code review requested label Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants