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.

Sign up for GitHub

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

Jump to bottom

Remove unnecessary process wait on MacOS (issue #1523) #1526

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Remove unnecessary process wait on MacOS (issue #1523) #1526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57e6f1f
Remove unnecessary process wait on MacOS (#1523)
cedel1 Aug 31, 2023
b34ada8
Remove unused code
cedel1 Aug 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions httpie/internal/daemons.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,7 @@ def _spawn_posix(args: List[str], process_context: ProcessContext) -> None:
if platform.system() == 'Darwin':
# Double-fork is not reliable on MacOS, so we'll use a subprocess
# to ensure the task is isolated properly.
process = _start_process(args, env=process_context)
# Unlike windows, since we already completed the fork procedure
# we can simply join the process and wait for it.
process.communicate()
Copy link
Author

@cedel1 cedel1 Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Waited for the process and could send some data to it or return its return code - but nothing of the sort is being done, basically blocking until the process finishes. But since the purpose of the process is to be daemonized (or is possibly daemonized already), it doesn't seem to make sense.

_start_process(args, env=process_context)
else:
os.environ.update(process_context)
with suppress(BaseException):
Expand Down
9 changes: 0 additions & 9 deletions tests/utils/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
"""Utilities for HTTPie test suite."""
import re
import shlex
import os
import sys
import time
Expand Down Expand Up @@ -205,16 +204,9 @@ class BaseCLIResponse:
devnull: str = None
json: dict = None
exit_status: ExitStatus = None
command: str = None
args: List[str] = []
complete_args: List[str] = []

@property
def command(self):
cmd = ' '.join(shlex.quote(arg) for arg in ['http', *self.args])
# pytest-httpbin to real httpbin.
return re.sub(r'127\.0\.0\.1:\d+', 'httpbin.org', cmd)

Comment on lines -212 to -217
Copy link
Author

@cedel1 cedel1 Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

pycodestyle (flake) complains that this redefines the command property a few lines above.

While it would be possible to correct that by removing the property from the list (and possibly adding setter and delete version of the @property to be able to change its value and/or delete the value), the only usage I found was a commented-out line a few lines below, so it made sense to me to just remove it completely, since it is basically unused code.

@classmethod
def from_raw_data(self, data: Union[str, bytes]) -> 'BaseCLIResponse':
if isinstance(data, bytes):
Expand Down Expand Up @@ -448,7 +440,6 @@ def dump_stderr():
if r.exit_status != ExitStatus.SUCCESS:
sys.stderr.write(r.stderr)

# print(f'\n\n$ {r.command}\n')
return r

finally:
Expand Down