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

database/sql: Why is errDBClosed not exported? #9947

Closed
chowey opened this issue Feb 20, 2015 · 9 comments
Closed

database/sql: Why is errDBClosed not exported? #9947

chowey opened this issue Feb 20, 2015 · 9 comments

Comments

@chowey
Copy link

chowey commented Feb 20, 2015

When I close a database connection and then try to execute a query on it, I get errDBClosed defined here:

http://golang.org/src/database/sql/sql.go#L630

Why is this variable not exported? It can be useful to do something like this:

_, err := db.Exec(`SELECT something`)
if err == sql.ErrDBClosed {
    return nil
}

The cost of exporting this variable is nothing.

@mikioh mikioh changed the title Why is errDBClosed not exported? database/sql: Why is errDBClosed not exported? Feb 21, 2015
@adg
Copy link
Contributor

adg commented Feb 22, 2015

Why should it be exported? What would you do differently?

@chowey
Copy link
Author

chowey commented Feb 22, 2015

I would use it like in my example above.

@parkr
Copy link

parkr commented Feb 22, 2015

This would be helpful in error handling.

@johto
Copy link
Contributor

johto commented Feb 22, 2015

IME most packages don't export their errCloseds, and this issue has been brought up before, e.g. #4373.

@adg
Copy link
Contributor

adg commented Feb 22, 2015

@chowey Why would you return nil instead of an error if the select fails? I'm looking for a bit more context, if you don't mind.

@chowey
Copy link
Author

chowey commented Feb 22, 2015

I'll explain my specific use case, so it is not so abstract.

I run a cooking process periodically on a separate goroutine. Occasionally this fails because of transaction locks, etc. My default behavior is to simply log the error and then retry during the next cook period.

When the program is asked to stop, I issue a db.Close(). This immediately causes any in-progress cooking process to fail with an error. The easy and robust way to deal with this is that the cooker understands this error message, stops its timer, does some fast cleanup and returns on its goroutine.

The current way I deal with it is to wrap db.Close() to save the "closed" state and then specifically check for the "closed" state when the cooker has an error, so it cleans up and exits gracefully at that point. This is a little more overhead but it adds up in complexity when I have multiple cookers.

Specifically I wouldn't return a nil instead of an error. The code looks more like this (or would look like this if errDBClosed was exported):

go func() {
    for {
        time.Sleep(time.Minute)
        _, err := db.Exec(`UPDATE something`)
        if err == sql.ErrDBClosed {
            // some fast cleanup code
            return
        } else {
            log.Printf("%v", err)
        }
    }
}()

@adg
Copy link
Contributor

adg commented Feb 22, 2015

My reading of the database/sql code might be wrong, but I think you can end up in a state where the database is closed because of an error, rather than you gracefully shutting it down. I think relying on db.Close as such a signal is probably not the right way to go.

You might try something like this, where all your workers can select on a timer or a quit channel, and your controller can signal shutdown with close(quit). That will shut your workers down immediately, instead of after the timeout, and you can also put a check in the select statement error handling to see whether you should report the error (I'd say you should anyway).

var quit chan struct{}

go func() {
    for {
        select {
        case <-quit:
            // some fast cleanup code
            return
        case <-time.After(time.Minute):
        }
        if _, err := db.Exec(`UPDATE something`); err != nil {
            log.Printf("%v", err)
        }
    }
}()

@chowey
Copy link
Author

chowey commented Feb 22, 2015

I'm certainly not arguing that there is no other way around this problem.

My question simply was, why is errDBClosed not exported?

Edit: Specifically, it seems to have been created with this commit:

41c5d8d#diff-ab2cf91bbf4757084fddc5afa8bf8f07R602

It looks like prior to this, the error was being reported using errors.New and the author of this commit decided to make it a variable because he uses it in two places instead of one. Probably it was done because it was minimal impact.

@rsc
Copy link
Contributor

rsc commented Apr 10, 2015

It is not exported because code is not supposed to check for it.

@rsc rsc closed this as completed Apr 10, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants