Skip to content

Fix crash on lazy initialization - #391

Merged
trishume merged 1 commit into
trishume:masterfrom
rhysd:issue-334
Nov 8, 2021
Merged

Fix crash on lazy initialization#391
trishume merged 1 commit into
trishume:masterfrom
rhysd:issue-334

Conversation

@rhysd

@rhysd rhysd commented Nov 3, 2021

Copy link
Copy Markdown
Contributor

Fixes #334
Closes #390

Problem

Crash at

self.regex.fill(regex).ok();
self.regex.borrow().unwrap()

Since this rarely happens in mutli-threads environment, I couldn't trace what happened. However, logically it would be:

  1. Thread1 starts .fill() call and acquires lock
  2. Thread2 starts .fill() call but cannot acquire lock (since Thread1 already did it)
  3. .fill() in Thread2 immediately returns Err(value) (but Thread2 ignores it by .ok())
  4. Thread2 calls .borrow(). However, at this point, the .fill() call in Thread1 is not completed yet. So .borrow() in Thread2 returns None
  5. The .unwrap() call crashes
  6. (Thread1 finishes to set value at .fill() call)

So,

cell.fill(something).ok();
cell.borrow().unwrap();

is not crash-free actually.

Solution

Using once_cell::sync::OnceCell instead of lazycell::ActomicLazyCell solves this issue.

https://docs.rs/once_cell/1.8.0/once_cell/sync/struct.OnceCell.html#method.get_or_init

Many threads may call get_or_init concurrently with different initializing functions, but it is guaranteed that only one function will be executed.

I also read its implementation. Initializing cell is synchronized with a queue.

https://github.com/matklad/once_cell/blob/7b2943b3828867a58690165961424230ca423403/src/imp_std.rs#L86-L107

@robinst robinst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM!

@rhysd rhysd changed the title Replace lazycell with once_cell to fix crash on lazy initialization Fix crash on lazy initialization Nov 3, 2021
@rhysd rhysd mentioned this pull request Nov 3, 2021

@Enselic Enselic left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have confirmed that this change works well with syntax lazy-loading (the sequel to #382). It also looks good in and of itself.

@trishume trishume left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for debugging this and fixing it!

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.

Panic in regex parser

4 participants