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

OnceLock: Add note about drop and statics #118581

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

ianrrees
Copy link
Contributor

@ianrrees ianrrees commented Dec 4, 2023

Hi! Just a minor documentation addition, I've attempted to build docs locally but ran in to issues, so am not 100% sure this change will render correctly.

@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Dec 4, 2023
Comment on lines 17 to 20
/// ‘lazy static’ or ‘memoizing’). Since [`drop`] is not called for static
/// items at the end of the program (see [the
/// reference](https://doc.rust-lang.org/reference/destructors.html), objects
/// stored using this pattern will not have their destructors called.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not necessarily against adding this somewhere in the documentation but why to OnceLock's example, of all places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT, the primary purpose of OnceLock is to manage a static item, and at least to me it wasn't obvious that a consequence of moving an item in to a (static) OnceLock that is that the item's destructor won't be called.

Copy link
Member

Choose a reason for hiding this comment

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

The codebase I work with has more uses of OnceCell and OnceLock in fields of random structs than in statics. A lot of statics are used to provide global mutable state which accumulates over time, which often requires something like Mutex or a thread_local RefCell.

Notably, we do mention this problem in the documentation of the static keyword.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting - I'm surprised to learn that OnceLock is commonly used in fields of random non-static structs, would be keen to see an example of that usage in the docs.

FWIW, my "systems" programming background is largely in C++, which does destruct statics. To me, OnceLock looks simply like a Rust implementation of a global singleton, and it was surprising that putting a struct in the idiomatic (or, at least following the example from docs) resulted in this behaviour where that object isn't dropped.

I appreciate that the docs already explain that statics aren't dropped; the point of the PR is to indicate that the docs around drop and static might be interesting to someone who's applying the example. IME static isn't a very widely used keyword, so a person who's looking at the OnceLock example probably isn't super familiar with the ramifications of using it.

Would a comment in the example code perhaps be a better place for this note?

Copy link
Member

Choose a reason for hiding this comment

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

If the entire struct has to be Sync for other reasons, it becomes natural to use OnceLock, because OnceCell is !Sync.

I'd be happy to see a comment in the examples, yeah, or even... hmm.

See, most of the Drops in std are "and this gets deallocated" (Vec) or "this gets advanced towards deallocation" (Arc), which means that once the program ends, it doesn't really matter: the OS sends all the types to the Big Drop Impl In The Sky. So I think this might be more motivated by a (perhaps slightly contrived?) example which presents something with a slightly-complicated Drop interacting with OnceCell/OnceLock in different ways. I suspect you encountered some sort of stumbling block that deserves more than just a sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue I encountered relates to a proprietary and rather crude Linux kernel-mode driver; basically the library I'm working on should make a call in to that driver to release a hardware resource, and that was done by destructing a Rust type. It's OK if abnormal program termination doesn't call drop on this type, because the "abnormal" aspect means we need to reset the whole system anyway, but it's problematic if normal termination doesn't release that resource.

But on a more general note, as before I found this surprising and bounced it off a couple other Rust folks (including in Discord) who didn't seem to know the right answer offhand either.

Will aim to update this PR later today with a code comment instead, thanks for your feedback!

Copy link
Member

Choose a reason for hiding this comment

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

Hm. I'm sorry, "abnormal program termination" doesn't actually mean anything to me, I'm a complete feral by systems programmer standards. I've seen people use it to refer specifically to abort(), but consider std::process::exit(exit_code) to be normal termination. I'm concerned about that statement, because that also will not call destructors.

Copy link
Contributor Author

@ianrrees ianrrees Dec 6, 2023

Choose a reason for hiding this comment

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

Sorry, I didn't mean anything technical/specific by "abnormal program termination".

If we imagine a spectrum of reasons why a program ended, with the extreme normal end "program finished its job", and "CPU exploded" as the extreme abnormal end; in this case MyStruct::drop() needs to be called only if the program finished its job. Anywhere else on the spectrum, the system is restarted. Or, I guess replaced if too far in the abnormal direction.

The issue here is that, by moving MyStruct in to the idiomatic (per docs) OnceLock usage, I have wound up with its drop never being called.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

...that's the first time I've ever seen THAT breakage on that CI action, I think.
Not something you have to fix.
...Pretty sure.

@ianrrees
Copy link
Contributor Author

ianrrees commented Dec 6, 2023

Rebased on to current master anyway

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

Ah, now tidy has a "real" complaint. Whitespace, I assume.

@ianrrees
Copy link
Contributor Author

ianrrees commented Dec 6, 2023

Fixed whitespace and rebased on to master.

@workingjubilee - what do you think about the example changes? The note felt a bit too forced with the HashMap example, but that example didn't give a very strong justification for the OnceLock.

@workingjubilee
Copy link
Member

I think it looks good, except instead of

/// // Do some slow/expensive computation here

you should probably use the fact that you can define code using stuff like

/// ```
/// # use hidden_import;

and the code on that line won't actually get rendered in the docs, so you can call an "off-screen" function that does nothing but has some sort of name that would be mildly stress-inducing to see if it was invoked inside for _ in 0..lots {}.

@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

Hehe, that's fair. :^)

@bors r+

@bors
Copy link
Contributor

bors commented Dec 8, 2023

📌 Commit 88fccc4 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 8, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#118505 (Elaborate on ip_addr bit conversion endianness)
 - rust-lang#118581 (OnceLock: Add note about drop and statics)
 - rust-lang#118677 ([rustdoc] Fix display of features)
 - rust-lang#118690 (coverage: Avoid unnecessary macros in unit tests)
 - rust-lang#118693 (Tell MirUsedCollector that the pointer alignment checks calls its panic symbol)
 - rust-lang#118695 (coverage: Merge refined spans in a separate final pass)
 - rust-lang#118709 (fix jobserver GLOBAL_CLIENT_CHECKED uninitialized before use)
 - rust-lang#118722 (rustdoc: remove unused parameter `reversed` from onEach(Lazy))

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 982a238 into rust-lang:master Dec 8, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 8, 2023
@bors
Copy link
Contributor

bors commented Dec 8, 2023

⌛ Testing commit 88fccc4 with merge 8043f62...

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 8, 2023
Rollup merge of rust-lang#118581 - ianrrees:add-drop-note-to-once_lock, r=workingjubilee

OnceLock: Add note about drop and statics

Hi!  Just a minor documentation addition, I've attempted to build docs locally but ran in to issues, so am not 100% sure this change will render correctly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants