Skip to content

Conversation

@sgkim126
Copy link
Contributor

@sgkim126 sgkim126 commented Dec 1, 2019

Currently, the main thread will exit while the snapshot service is creating snapshots.

@remagpie
Copy link
Contributor

remagpie commented Dec 2, 2019

I think we merged a similar implementation in #1869 (in snapshot branch).
Would you check it first?

}
joins.lock().remove(&id);
});
self.joins.lock().insert(id, join);
Copy link
Contributor

Choose a reason for hiding this comment

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

There might be a race condition where the spawned thread exits first, so joins.remove() is called first and joins.insert() is called later.

How about cleaning it lazily?

fn new_blocks() {
	...
	let arc = Arc::new(());
	let weak = Arc::downgrade(&arc);
	let join = spawn(move || {
		let arc = arc; // If the stack dies, the arc dies. 
	    ...
	}
	let mut joins = joins.lock();
	joins.push((weak, join));
	joins.retain(|(weak, _)| Weak::upgrade(weak).is_some());
	...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can be solved by locking self.joins in the upper line.

let joins = Arc::clone(&self.joins);
let self_join = self.joins.lock();
let join = spawn(move || { ... });
self_join.insert(id, join);

The variable name is a bit awkward, but I think you'll get the point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, @remagpie's looks simple. @foriequal0 how about it?

@sgkim126
Copy link
Contributor Author

sgkim126 commented Dec 2, 2019

I think we merged a similar implementation in #1869 (in snapshot branch).
Would you check it first?

Ah. Yes, it is fixed in the snapshot branch. I'll close it.

@sgkim126 sgkim126 added the wontfix This will not be worked on label Dec 2, 2019
@sgkim126 sgkim126 closed this Dec 2, 2019
@sgkim126 sgkim126 deleted the snapshot branch December 2, 2019 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sync wontfix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants