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

Replace std::io::lazy::Lazy with a copy of lazy-static's similar, but Once-using, type. #53646

Closed
eddyb opened this issue Aug 23, 2018 · 7 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@eddyb
Copy link
Member

eddyb commented Aug 23, 2018

It's not clear why std::io::lazy doesn't use Once, as that was available at the time when it was introduced.

#53108 marked Lazy::new as unsafe because of the lack of reentrance safety in the mutex being used, but Once specifically doesn't have reentrancy issues.

lazy_static version: https://github.com/rust-lang-nursery/lazy-static.rs/blob/13159c2b615903d93b4cd4f98d17fdfe70392a3a/src/inline_lazy.rs

However, we need to change it per rust-lang-nursery/lazy-static.rs#117, otherwise it's unsound.

cc @RalfJung @alexcrichton

@eddyb eddyb added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 23, 2018
@RalfJung
Copy link
Member

There was also some discussion on IRL to move lazy-static into libstd and claims that it is not necessary with const fn. Why can't this use Once more directly and entirely safely?

@RalfJung
Copy link
Member

#53604 changes Lazy to be even more similar to Once, so I guess changing it all the way should be trivial now...

In fact, @oli-obk why wasn't that changed as part of that PR? Just to keep the changes smaller?

@oli-obk
Copy link
Contributor

oli-obk commented Aug 28, 2018

Just to keep the changes smaller?

yes

@RalfJung
Copy link
Member

Lazy does some magic to also register an at-exit hook for deallocation:

        // If we successfully register an at exit handler, then we cache the
        // `Arc` allocation in our own internal box (it will get deallocated by
        // the at exit handler). Otherwise we just return the freshly allocated
        // `Arc`.
        let registered = sys_common::at_exit(move || {
            let ptr = {
                let _guard = self.lock.lock();
                self.ptr.replace(done())
            };
            drop(Box::from_raw(ptr))
        });

Maybe that's why it can't just use Once. self.ptr is actually written to twice: once to initialize, and once to de-initialize.

@workingjubilee
Copy link
Member

We stabilized LazyLock and LazyCell. I think we're done here.

@RalfJung
Copy link
Member

This issue wasn't about stabilizing anything, it was entirely about std internals... turns out this was fixed 4 years ago on bab15f7.

@workingjubilee
Copy link
Member

Yes, "we stabilized these" was, in this case, short for "we have refactored almost every single instance of Lazy-whatever in the standard library."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants