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

SessionClient has no attribute _terminate_on_server #100

Open
astrale-sharp opened this issue Mar 28, 2024 · 12 comments
Open

SessionClient has no attribute _terminate_on_server #100

astrale-sharp opened this issue Mar 28, 2024 · 12 comments
Labels

Comments

@astrale-sharp
Copy link

Hello there, not sure exactly why I run into this code in the first place but

# ./core/session_client.py:254
    def terminate(
        self,
        signum: Optional[int] = None,  # Only required for for signal handling.
        frame: Optional[FrameType] = None,  # Only required for for signal handling.
    ):

        if not self._client_up:
            return

        self._log.info("[session-client] TERMINATING ...")

        try:
            self._terminate_on_server() <--- this here
        except EOFError:  # EOFError is raised if server socket is closed - ignore it
            self._log.info("[session-client] Remote socket closed.")
..........

grepping shows that _terminate_on_server doesn't exist in the code base!

@astrale-sharp
Copy link
Author

Happens when I call from zugbruecke.ctypes import windll somewhere ;)

@astrale-sharp
Copy link
Author

astrale-sharp commented Mar 28, 2024

For some reason wine is not in my PATH anymore which surely didn't help:

EDIT: having it in my path again makes the code not go to the same path again

@s-m-e
Copy link
Member

s-m-e commented Mar 28, 2024

Hi @astrale-sharp can this be marked as resolved them?

@astrale-sharp
Copy link
Author

I mean, I don't have an issue anymore but isn't it a bit akward to have a undefined function there? 😇

@s-m-e
Copy link
Member

s-m-e commented Mar 28, 2024

You are right - this function is created if Wine is up and running. If not, it does not exist. This needs better error handling.

@s-m-e s-m-e added the bug label Mar 28, 2024
@astrale-sharp
Copy link
Author

We could check for the presence of the function and Raise an error hinting at wine being not present? I don't mind doing a quick PR either ;)

@s-m-e
Copy link
Member

s-m-e commented Mar 29, 2024

I'd happily accept a PR. Though, if Wine is not present, a session should fail to start much earlier, not in this code path - this kind of failure mode is unusual. Can you reproduce this issue?

@astrale-sharp
Copy link
Author

quick hack : sudo chmod -x /usr/bin/wine* /opt/wine-stable/bin/wine*

Then

❯ python3
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from zugbruecke.ctypes import windll
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/ctypes/__init__.py", line 44, in <module>
    _session = _CtypesSession()
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session.py", line 129, in __init__
    self._current_session = SessionClient(config=Config(**kwargs))
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session_client.py", line 116, in __init__
    self._wait_for_server_status_change(target_status=True)
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session_client.py", line 328, in _wait_for_server_status_change
    raise TimeoutError("session server did not appear")
TimeoutError: session server did not appear
>>> 
@typechecked
class Config(dict, ConfigABC):
.....
    def __getitem__(self, key: str) -> Any:
.....
# zugbruecke/core/config.py:147
        if key == "pythonversion":
            return PythonVersion(self['arch'], 3, 7, 4, 'stable')  # Define Wine-Python version

If I understand correctly, this call to PythonVersion will verify that the arch is known and the version is valid but not that wine is accessible which the Config should maybe do at this point?

@astrale-sharp
Copy link
Author

On second thought
Env::ensure() seems to be called
in turn calling setup_wineprefix

Then we stumble upon this

# wenv/_core/env.py:394
proc = subprocess.Popen(["wine", "wineboot", "-i"], env=envvar_dict)
        proc.wait()
        if proc.returncode != 0:
            sys.exit(1)

Instead of just exiting with an error code here we could also print a warning hinting at the absence of wine

@astrale-sharp
Copy link
Author

in setup_wineprefix we're actually returning before starting the process here

and the process initialize wine if in overwrite mode.. maybe this would be a good place to check if wine is in the path anyway? either in setup_wineprefix or just before calling it

@astrale-sharp
Copy link
Author

ensure definitely feels like the place where function could return error code or just have an additional check and error reporting, although setup_zugbruecke seems like a good candidate too.

I think I'd try to Popen(["wenv" | "wine", "version"]) and report an error + abort if it returns an error code

last message from this intense spamming I swear 😅

@s-m-e
Copy link
Member

s-m-e commented Mar 30, 2024

quick hack : sudo chmod -x /usr/bin/wine* /opt/wine-stable/bin/wine*

Then

❯ python3
Python 3.10.12 (main, Nov 20 2023, 15:14:05) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from zugbruecke.ctypes import windll
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/ctypes/__init__.py", line 44, in <module>
    _session = _CtypesSession()
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session.py", line 129, in __init__
    self._current_session = SessionClient(config=Config(**kwargs))
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session_client.py", line 116, in __init__
    self._wait_for_server_status_change(target_status=True)
  File "/home/astrale/.local/lib/python3.10/site-packages/zugbruecke/core/session_client.py", line 328, in _wait_for_server_status_change
    raise TimeoutError("session server did not appear")
TimeoutError: session server did not appear
>>> 

That's actually the intended behavior, a TimeoutError. Python on Wine and Wine itself can fail in to many different ways (or require ages to start) that a time out seemed like the most sensible thing to do to capture a failure of the server component.

Your initial bug report suggested that you got an attribute error in the shutdown sequence instead, caused by a missing _terminate_on_server, i.e. a shutdown on an incompletely loaded server component. I am still not completely sure what chain of events would cause this to happen. This is what I would love to understand before I or you for that matter attempt a fix.

@typechecked
class Config(dict, ConfigABC):
.....
    def __getitem__(self, key: str) -> Any:
.....
# zugbruecke/core/config.py:147
        if key == "pythonversion":
            return PythonVersion(self['arch'], 3, 7, 4, 'stable')  # Define Wine-Python version

If I understand correctly, this call to PythonVersion will verify that the arch is known and the version is valid but not that wine is accessible which the Config should maybe do at this point?

Yep, correct. This mechanism is the foundation for fetching CPython for Windows (based on the specified version) if the version & arch is valid (i.e. there is a build that can be pulled).

Instead of just exiting with an error code here we could also print a warning hinting at the absence of wine

Agreed. Env::ensure() should do a better job overall.

I think I'd try to Popen(["wenv" | "wine", "version"]) and report an error + abort if it returns an error code

I'd be careful with that. I'd rather check if wine is in path and executable. Starting Wine, even if its only with --version, can have wild consequences (plenty of bad experience).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants