Skip to content

fix crash on shutdown when e.g. changing -txindex and abort action#6282

Merged
laanwj merged 1 commit intobitcoin:masterfrom
Diapolo:fix_shutdown
Jun 18, 2015
Merged

fix crash on shutdown when e.g. changing -txindex and abort action#6282
laanwj merged 1 commit intobitcoin:masterfrom
Diapolo:fix_shutdown

Conversation

@Diapolo
Copy link

@Diapolo Diapolo commented Jun 15, 2015

@Diapolo Diapolo mentioned this pull request Jun 15, 2015
Closed
laanwj
laanwj reviewed Jun 15, 2015
View reviewed changes
src/wallet/db.h Outdated
Copy link
Member

@laanwj laanwj Jun 15, 2015

Choose a reason for hiding this comment

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

Shouldn't this field go away, then?

Copy link
Author

@Diapolo Diapolo Jun 15, 2015

Choose a reason for hiding this comment

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

I didn't replace all occurances, because it's used when building paths still.

Copy link
Member

@laanwj laanwj Jun 15, 2015

Choose a reason for hiding this comment

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

Do add a comment then, on why the duplicated field was added. Otherwise people will be really confused to read this code.

@laanwj
Copy link
Member

laanwj commented Jun 15, 2015

If boost::path gives any problems at static deinitialization, I still dont feel good with this.

My suggestion would be to precompute all the usages of path in CDBEnv::Open. E.g.

std::string strPath;
std::string strPathLogDir;

Then in the initialization

strPath = pathIn.string();
strPathLogDir = pathLogDir.string();

Remove the boost::filesystem::path path field. Replace the usages of path outside CDBEnv::Open with these precomputed strings.

@laanwj laanwj added the Bug label Jun 15, 2015
@Diapolo
Copy link
Author

Diapolo commented Jun 15, 2015

@laanwj That suggestion is fine, I tried to be minimal invasive but will gladly remove the path field.

@Diapolo
Copy link
Author

Diapolo commented Jun 15, 2015

@laanwj See pull after rebase, I removed the path field, but as quite some functions require a boost::filesystem::path I couldn't eliminate all uses (e.g. TryCreateDirectory() and boost::filesystem::remove_all()). And also I didn't want to play with the / operator used in boost::path as dir separator, so I didn't replace it with the string /, which could cause problems on certain OSes, where a native path separator is perhaps different.

@Diapolo
Copy link
Author

Diapolo commented Jun 15, 2015

Seems the build faulire is unrelated to the pull: "No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.

The build has been terminated"

@laanwj
Copy link
Member

laanwj commented Jun 15, 2015

Looks good to me, utACK, travis error is the misbehaving comparison tool again...

@Diapolo
Copy link
Author

Diapolo commented Jun 16, 2015

Now it passed, great!

@laanwj
Copy link
Member

laanwj commented Jun 16, 2015

I removed the path field, but as quite some functions require a boost::filesystem::path I couldn't eliminate all uses

Right. Eliminating the uses of boost::filesystem is not the goal here, just avoiding that a static filesystem::path field stays around as part of CDBEnv, which this does now.

laanwj
laanwj reviewed Jun 16, 2015
View reviewed changes
src/wallet/db.h Outdated
Copy link
Member

@laanwj laanwj Jun 16, 2015

Choose a reason for hiding this comment

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

Please add a comment here why this field is a string instead of a boost::filesystem::path, so that a future smart-ass doesn't think "hey let's use that type directly" and changes it back.

Copy link
Author

@Diapolo Diapolo Jun 16, 2015

Choose a reason for hiding this comment

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

Done!

fix crash on shutdown when e.g. changing -txindex and abort action
0ce30ea 
- fixes #3136
- the problem is related to Boost path and a static initialized internal
  pointer
- using a std::string in CDBEnv::EnvShutdown() prevents the problem
- this removes the boost::filesystem::path path field from CDBEnv
@sipa
Copy link
Member

sipa commented Jun 16, 2015

utACK

@laanwj laanwj merged commit 0ce30ea into bitcoin:master Jun 18, 2015
laanwj added a commit that referenced this pull request Jun 18, 2015
@laanwj
Merge pull request #6282
cbec57f 
0ce30ea fix crash on shutdown when e.g. changing -txindex and abort action (Philip Kaufmann)
@Diapolo Diapolo deleted the fix_shutdown branch June 19, 2015 13:35
@Diapolo Diapolo mentioned this pull request Jun 19, 2015
Closed
laanwj pushed a commit that referenced this pull request Jun 23, 2015
@laanwj
fix crash on shutdown when e.g. changing -txindex and abort action
daf956b 
- fixes #3136
- the problem is related to Boost path and a static initialized internal
  pointer
- using a std::string in CDBEnv::EnvShutdown() prevents the problem
- this removes the boost::filesystem::path path field from CDBEnv

Github-Pull: #6282
Rebased-From: 0ce30ea
@laanwj
Copy link
Member

laanwj commented Jun 23, 2015

Backported to 0.11 as daf956b, thanks @luke-jr for the heads up

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Qt] Do you want to rebuild the block database now? No -> crash

3 participants

@Diapolo @laanwj @sipa