[go: up one dir, main page]
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

Partial fix: Callable alias can provoke Bad file descriptor exception in the internal or external code #5645

Merged
merged 24 commits into from
Aug 9, 2024
Merged

Partial fix: Callable alias can provoke Bad file descriptor exception in the internal or external code #5645

merged 24 commits into from
Aug 9, 2024

Conversation

anki-code
Copy link
Member
@anki-code anki-code commented Aug 6, 2024 •
edited
Loading

Motivation

Closes #5631. After huge amount of experiments and tests in #5640 I see that it looks like during #3897 the handlers that should be closed were mixed up.

Then after tests on POSIX and on Windows I see that we need to split the logic:

  • For POSIX we need to close stdout/stderr for callable alias and this is completely solves the issue for POSIX.
  • For Windows we need to keep the logic of closing the thread stdout/stderr the same.

Before

See many kind of errors in #5631 (comment)

The minimal cases to repeat in xonsh --no-rc -st rl:

$XONSH_SHOW_TRACEBACK = True
aliases['t'] = 'echo 1 && echo 2'

for i in range(0, 30):
    t

# OSError: [Errno 9] Bad file descriptor
$XONSH_SHOW_TRACEBACK = True
@aliases.register
def _e(a,i,o,e):
    echo -n O
    echo -n E 1>2
    execx("echo -n O")
    execx("echo -n E 1>2")
    print("o")
    print("O", file=o)
    print("E", file=e)

for i in range(0, 30):
    echo -n 2
    print($(e), !(e), $[e], ![e])

# OSError: [Errno 9] Bad file descriptor

After

Running the code above is stable.

Notes

For community

⬇️ Please click the 👍 reaction instead of leaving a +1 or 👍 comment

@anki-code anki-code changed the title draft: fix bad file descriptor 3 Fix: Callable alias can provoke Bad file descriptor exception in the internal or external code Aug 6, 2024
@anki-code anki-code mentioned this pull request Aug 6, 2024
Closed
@anki-code anki-code marked this pull request as ready for review August 6, 2024 22:15
This was referenced Aug 6, 2024
Open
Merged
jnoortheen
jnoortheen previously approved these changes Aug 7, 2024
View reviewed changes
@anki-code anki-code changed the title Fix: Callable alias can provoke Bad file descriptor exception in the internal or external code Fix: Callable alias can provoke Bad file descriptor exception in the internal or external code Aug 7, 2024
@anki-code anki-code mentioned this pull request Aug 7, 2024
Open
@anki-code
Copy link
Member Author

Thanks @jnoortheen!
@gforsyth @jaraco please test this and approve if it works to you.

jaraco
jaraco previously approved these changes Aug 7, 2024
View reviewed changes
Copy link
Collaborator
@jaraco jaraco 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 great for me on my main mac workstation. I installed it with:

pipx runpip xonsh install git+https://github.com/xonsh/xonsh@refs/pull/5645/head

Then I launched the new shell, ran some aliases, and everything seems to be working as expected.

gforsyth
gforsyth previously approved these changes Aug 7, 2024
View reviewed changes
Copy link
Collaborator
@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

excellent detective work, @anki-code .

@gforsyth
Copy link
Collaborator
gforsyth commented Aug 7, 2024

It looks like the ubuntu 3.12 job failed with a timeout -- could be a deadlock, could be a fluke

@anki-code anki-code dismissed stale reviews from gforsyth and jaraco via ee9eb7e August 7, 2024 14:47
@anki-code
Copy link
Member Author

Thanks to xore team!
I'm waiting a bit the reports from @amacfie and @rickysarraf who focused the issue in Discussions.

@anki-code
Copy link
Member Author

It looks like the ubuntu 3.12 job failed with a timeout -- could be a deadlock, could be a fluke

I see the same after restarting. I'll take a look. Thanks!

@anki-code anki-code marked this pull request as draft August 8, 2024 09:27
@anki-code anki-code marked this pull request as ready for review August 8, 2024 13:07
@anki-code
Copy link
Member Author
anki-code commented Aug 8, 2024 •
edited
Loading

It looks like the ubuntu 3.12 job failed with a timeout -- could be a deadlock, could be a fluke

I see the same after restarting. I'll take a look. Thanks!

  1. I tested this on Ubuntu+3.12 and all tests is working: 🟢

    podman run --rm -it python:3.12-slim /bin/bash -c "apt update && apt install -y git && pip install -U git+https://github.com/xonsh/xonsh@fix_bad_file_descriptor && xonsh"

  2. So after increasing the timeout the CI tests also passed. 🟢

  3. The final thing I'm waiting is the feedback here - ExecAlias: Bad file descriptor #5622 (reply in thread) 🟡

@anki-code anki-code changed the title Fix: Callable alias can provoke Bad file descriptor exception in the internal or external code Partial fix: Callable alias can provoke Bad file descriptor exception in the internal or external code Aug 8, 2024
gforsyth
gforsyth previously approved these changes Aug 8, 2024
View reviewed changes
@anki-code anki-code added this to the 0.18.3 milestone Aug 8, 2024
@anki-code
Copy link
Member Author
anki-code commented Aug 9, 2024 •
edited
Loading

Next steps:

  1. After tests and feedback I see that this PR fixes exception in case of no nesting of callable alias. I'm going to merge it and we will release it in 0.18.3 to reduce errors in ExecAlias.

  2. I prepared more complex PR Switch stdout, stderr to single open in b-mode to solve Bad file descriptor exception in callable alias #5656 that looks like complete solving of Callable alias can provoke Bad file descriptor exception in the internal or external code #5631.
    We need to review and test it more deeply. So my suggestion is to release it in 0.18.4.

Thanks for review to xore!

@anki-code anki-code merged commit 938db85 into main Aug 9, 2024
12 checks passed
@anki-code anki-code mentioned this pull request Aug 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnoortheen jnoortheen jnoortheen left review comments

@gforsyth gforsyth gforsyth left review comments

@jaraco jaraco jaraco left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.18.3
Development

Successfully merging this pull request may close these issues.

Callable alias can provoke Bad file descriptor exception in the internal or external code
4 participants
@anki-code @gforsyth @jaraco @jnoortheen