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.

Sign up for GitHub

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

Adjust error for already locked db and prevent level db lock on malformed connstr #18923

Merged
merged 5 commits into from
Feb 27, 2022
Merged

Adjust error for already locked db and prevent level db lock on malformed connstr #18923

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a5677e8
Adjust error for already locked db and prevent level db lock on malfo…
zeripath Feb 26, 2022
2a52daf
Merge branch 'main' into improve-warning-for-wrapped-queues
6543 Feb 26, 2022
4e58ab2
Merge branch 'main' into improve-warning-for-wrapped-queues
zeripath Feb 27, 2022
b125ad2
On close also check datadir
zeripath Feb 27, 2022
58ffaa4
Merge branch 'main' into improve-warning-for-wrapped-queues
zeripath Feb 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 32 additions & 4 deletions modules/nosql/manager_leveldb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
package nosql

import (
"fmt"
"path"
"strconv"
"strings"

"code.gitea.io/gitea/modules/log"
"github.com/syndtr/goleveldb/leveldb"
"github.com/syndtr/goleveldb/leveldb/errors"
"github.com/syndtr/goleveldb/leveldb/opt"
Expand Down Expand Up @@ -40,6 +42,12 @@ func (m *Manager) CloseLevelDB(connection string) error {

// GetLevelDB gets a levelDB for a particular connection
func (m *Manager) GetLevelDB(connection string) (*leveldb.DB, error) {
// Convert the provided connection description to the common format
uri := ToLevelDBURI(connection)

// Get the datadir
dataDir := path.Join(uri.Host, uri.Path)

m.mutex.Lock()
defer m.mutex.Unlock()
db, ok := m.LevelDBConnections[connection]
Expand All @@ -48,12 +56,28 @@ func (m *Manager) GetLevelDB(connection string) (*leveldb.DB, error) {

return db.db, nil
}
uri := ToLevelDBURI(connection)

db, ok = m.LevelDBConnections[uri.String()]
if ok {
db.count++

return db.db, nil
}

// if there is already a connection to this leveldb reuse that
// NOTE: if there differing options then only the first leveldb connection will be used
db, ok = m.LevelDBConnections[dataDir]
if ok {
db.count++
log.Warn("Duplicate connnection to level db: %s with different connection strings. Initial connection: %s. This connection: %s", dataDir, db.name[0], connection)
db.name = append(db.name, connection)
m.LevelDBConnections[connection] = db
Comment on lines +81 to +82
Copy link
Contributor

@wxiaoguang wxiaoguang Feb 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
db.name = append(db.name, connection)
m.LevelDBConnections[connection] = db

If we run into this if, then neither connection nor uri exists in the LevelDBConnections.

But here the code only append the connection to the name list, but no uri, is it by purpose?

In my opinion:

  • Either we keep consistent that we append both non-existing connection and uri into the name list
  • Or append nothing (remove these 2 lines), because the logic is sill correct, no performance degrade, and more simple.

Since the old code works, this comment is not a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It prevents the repeated logging of the same message

Copy link
Contributor

Choose a reason for hiding this comment

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

But it's not complete.

Current code It only prevents repeated logging for the new connection, but not for the new uri with the same directory. (that the case 1)

And I do not think there will be repeated loggings, the queues call GetLevelDB for open the db, there seems no repeated calls (so that why case 2 is also fine because there seems to be no repeated log messages)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it absolutely does prevent repeated logging of the same message. The log message doesn't contain the inferred URI. The point is to tell the server user which strings in their configuration are incorrect.

There are multiple queues. Depending on the order of initialisation and state of the incorrect configuration without this you face repeatedly seeing the same warning message.

return db.db, nil
}
db = &levelDBHolder{
name: []string{connection, uri.String()},
name: []string{connection, uri.String(), dataDir},
}

dataDir := path.Join(uri.Host, uri.Path)
opts := &opt.Options{}
for k, v := range uri.Query() {
switch replacer.Replace(strings.ToLower(k)) {
Expand Down Expand Up @@ -134,7 +158,11 @@ func (m *Manager) GetLevelDB(connection string) (*leveldb.DB, error) {
db.db, err = leveldb.OpenFile(dataDir, opts)
if err != nil {
if !errors.IsCorrupted(err) {
return nil, err
if strings.Contains(err.Error(), "resource temporarily unavailable") {
return nil, fmt.Errorf("unable to lock level db at %s: %w", dataDir, err)
}

return nil, fmt.Errorf("unable to open level db at %s: %w", dataDir, err)
}
db.db, err = leveldb.RecoverFile(dataDir, opts)
if err != nil {
Expand Down