Closer behavior

[yolocs]

https://github.com/abcxyz/jvs/blob/c74e145411363aed1d245d5530ed92f246df1abe/pkg/cli/public_key_server.go#L67-L75

In case of error, we will not call closer().

https://github.com/abcxyz/jvs/blob/c74e145411363aed1d245d5530ed92f246df1abe/pkg/cli/public_key_server.go#L100-L118

This means if kmsClient was created but we failed to create render, we will not close the kmsClient.

That feels like a bug but in practice should be OK. An error from RunUnstarted means we haven't really started so there isn't risk from half-processed requests.

What are your thoughts?

cc: @sethvargo @sqin2019

[sethvargo]

The risk is more about leaving established network connections open.

[yolocs]

The risk is more about leaving established network connections open.

Usually we exit the program on such error cases. So you mean we should fix this?

[sqin2019]

Fix should be straight forward like below?

        defer closer() 
    if err != nil { 
        return err 
    } 

[yolocs]

@sqin2019 Yes and no. Most of the code will only return a valid closer if there is no error. It's extremely common to write code like the following instead of calling defer first:

closer, err := doSomething()
if err != nil {
  return err
}
defer closer()

[yolocs]

I'm going to try a multicloser idea. Stay tuned.

[yolocs]

POC in: https://github.com/abcxyz/pkg/pull/135

I essentially turned @sqin2019 's idea into a lib

[yolocs]

This should already be fixed.