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

fix segfault on watch test, #58 #59

Merged
merged 3 commits into from
May 4, 2018
Merged

Conversation

yjh0502
Copy link
Collaborator

@yjh0502 yjh0502 commented May 3, 2018

The segfault (#58) happens when watch outlives database/cluster. It happens befause TrxWatch does not keep any refcount to database/cluster. I fixed the issue by making all FdbFuture live with either Transaction or Database.

I found another possible problem while fixing the issue, which is caused by drop order of Rust. Rust drops struct field from top to bottom[1]. If we define Cluster prior to Database, cluster will be dropped before Database, which might cause a problem. valgrind does not complain for this case but I changed the field order to just make sure.

[1] https://github.com/rust-lang/rfcs/blob/master/text/1857-stabilize-drop-order.md#tuples-structs-and-enum-variants

@yjh0502 yjh0502 requested a review from bluejekyll May 3, 2018 11:05
@bluejekyll
Copy link
Collaborator

bluejekyll commented May 3, 2018

Rust drops struct field from top to bottom[1].

I don't believe that linked RFC can be relied on, and it doesn't look like there is a path forward on this: rust-lang/rfcs#1857


Edit: I just reread that, and it was merged... it looks like it's a non-change, meaning they will be keeping drop order. I completely missed this, and thought something happened related to struct packing.

Copy link
Collaborator

@bluejekyll bluejekyll left a comment

Choose a reason for hiding this comment

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

Ignore my previous comments, my understanding of drop order was outdated.

This looks good, if a little confusing. We basically will now have two Futures, one scoped to Database, and one to Transaction, as I understand it. Which seems logical.

@bluejekyll bluejekyll merged commit e83a953 into Clikengo:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants