Skip to content

redirect fds rather than closing#10028

Merged
skshetry merged 1 commit into
treeverse:mainfrom
skshetry:fix-daemon
Oct 18, 2023
Merged

redirect fds rather than closing#10028
skshetry merged 1 commit into
treeverse:mainfrom
skshetry:fix-daemon

Conversation

@skshetry
Copy link
Copy Markdown
Collaborator

On inherited processes, the sys.{stdin/stderr/stdout} object were being set to None due to result of closing the standard stream fds (see closerange below). We redirect 0, 1 and 2 to dev null now.

Also rolled back to use subprocess for macOS as fork is not safe there.

I have also added a dvc daemon test command for use in a functional test.

@skshetry skshetry changed the title redirect fds rather than closing the fds redirect fds rather than closing Oct 17, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 17, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Files Coverage Δ
dvc/commands/daemon.py 72.72% <100.00%> (+6.06%) ⬆️
tests/func/test_daemon.py 86.66% <86.66%> (ø)
dvc/daemon.py 48.05% <18.18%> (ø)

... and 67 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

Copy link
Copy Markdown
Contributor

@dberenbaum dberenbaum left a comment

Choose a reason for hiding this comment

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

It's working as expected for me

Comment thread dvc/daemon.py
Comment thread dvc/daemon.py Outdated
_suppress_resource_warning(popen)


def redirect_streams_to_null():
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
def redirect_streams_to_null():
def _redirect_streams_to_null():

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's at least include a link to your explanation comment on github.

Comment thread dvc/commands/daemon.py Outdated
@efiop
Copy link
Copy Markdown
Contributor

efiop commented Oct 17, 2023

@skshetry From your research in #10026 (comment) , are there indications that fork might be a problem on linux as well?

@efiop efiop added the bugfix fixes bug label Oct 18, 2023
@skshetry
Copy link
Copy Markdown
Collaborator Author

I haven't found any issues with fork (without exec) in Linux, except on multithreading cases. But it seems fork is not considered safe in Unix if you don't immediately exec and might cause deadlock. See https://www.evanjones.ca/fork-is-dangerous.html.

I am not sure if using fork only on Linux is worth it at this point, if we are not going to use it in Windows and in macOS.
I will create a PR to use subprocess instead.

@efiop
Copy link
Copy Markdown
Contributor

efiop commented Oct 18, 2023

@skshetry Sounds good. Do you want to fix windows test so we could merge this for now or would you like to wait for that PR?

@skshetry
Copy link
Copy Markdown
Collaborator Author

I will wait for the next PR, i need to add tests. Will keep this open as an alternative for now.

@skshetry
Copy link
Copy Markdown
Collaborator Author

Unfortunately, we'll have to stick with this PR. There is no way to detach a subprocess (without double-forking).

@skshetry skshetry merged commit c39ce61 into treeverse:main Oct 18, 2023
@skshetry skshetry deleted the fix-daemon branch October 18, 2023 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugfix fixes bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants