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

Better error when compiled artifacts are in use #257

Closed
davidhewitt opened this issue Jun 29, 2022 · 7 comments · Fixed by #354
Closed

Better error when compiled artifacts are in use #257

davidhewitt opened this issue Jun 29, 2022 · 7 comments · Fixed by #354

Comments

@davidhewitt
Copy link
Member

I was thinking the other day it might be nice to check if any processes are using the installed extension before attempting to copy across.

I think on Windows we'll get an OsError raised during copy, and on Unix any running processes are just likely to crash when they next need to load a segment of the native code.

Doing a check and raising our own friendly error might be nicer UX.

@adamreichold
Copy link
Member

adamreichold commented Jun 29, 2022

on Unix any running processes are just likely to crash when they next need to load a segment of the native code.

I think on Unix, you will get ETXTBSY if you modify a currently running executable in place:

loop-forever> ./target/debug/loop-forever >/dev/null &
[1] 22032
loop-forever> echo "foobar" > target/debug/loop-forever
bash: target/debug/loop-forever: Text file busy

Though this is not completely reliable and might be removed in the future, c.f. https://lwn.net/Articles/866493/

Depending on how copying files is actually done, this might also not be a problem at all, i.e. if the new file is moved to replace the old one, then there is no issue as the old file sticks around until no more process has it opened.

@davidhewitt
Copy link
Member Author

Oh, good to know. I think when I've seen segfaults before it was for libraries rather than executables. Might behave differently?

This is just a "nice to have" thing for development so I probably won't try to implement this any time soon. Just a passing thought.

@adamreichold
Copy link
Member

I think when I've seen segfaults before it was for libraries rather than executables. Might behave differently?

I think this mostly happens because updates are not transactional, e.g. an older executable will after having been started dynamically load a newer library that has an different ABI the older version of the library. But as written above, ETXTBSY was never completely reliable.

@davidhewitt
Copy link
Member Author

From the article you linked above:

The MAP_DENYWRITE flag was created for just this use case, so that shared libraries could not be written while in use. When MAP_DENYWRITE went away, so did that protection; current Linux systems will happily allow a suitably privileged user to overwrite in-use, shared libraries.

It really does sound to me like shared libraries don't get ETXTBSY protection 😊

@adamreichold
Copy link
Member

Indeed, the removal seems to be further along than I remembered from reading the article. Appears like I did not read it very thoroughly. 😊

@adamreichold
Copy link
Member

So for extensions modules which are shared libraries ETXTBSY is irrelevant. Sorry for the noise...

@davidhewitt
Copy link
Member Author

Not at all! Always worth sharing information and debating my spurious claims of segfaults 😂

erikjohnston added a commit to erikjohnston/setuptools-rust that referenced this issue Sep 29, 2022
When the built shared library is copied to the correct location, it
actually *modifies* any existing shared library, which causes any
running process using it to segfault.

To fix this, we first delete any existing shared library (which is safe
to do) and then copy the newly built shared library to the correct
location as a new file.

Related to PyO3#257
davidhewitt pushed a commit that referenced this issue Oct 4, 2022
* Fix segfaults caused by modifying existing shared library

When the built shared library is copied to the correct location, it
actually *modifies* any existing shared library, which causes any
running process using it to segfault.

To fix this, we first delete any existing shared library (which is safe
to do) and then copy the newly built shared library to the correct
location as a new file.

Related to #257

* Use os.rename

* Use os.replace

* Update setuptools_rust/build.py

Co-authored-by: Adam Reichold <adamreichold@users.noreply.github.com>

* Update changelog

Co-authored-by: Adam Reichold <adamreichold@users.noreply.github.com>
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 a pull request may close this issue.

2 participants