GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add test for preexec_fn fd double close issue by fantix · Pull Request #479 · MagicStack/uvloop

Conversation

@fantix

@fantix fantix commented Jul 17, 2022

Copy link
Copy Markdown
Member

Refs #466

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@fantix
Add test for preexec_fn fd double close issue
b1f4f3d
@fantix

fantix commented Jul 17, 2022

Copy link
Copy Markdown
Member Author

@graingert may I ask for a review pls?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
graingert
graingert reviewed Jul 17, 2022
View reviewed changes
Comment thread tests/test_process.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@fantix @graingert
Update tests/test_process.py
757e39c 
Co-authored-by: Thomas Grainger <[email protected]>
graingert
graingert reviewed Jul 17, 2022
View reviewed changes
Comment thread tests/test_process.py
asyncio.run(test())
fut.result()
""")
subprocess.check_call([sys.executable, '-c', script])

@graingert graingert Jul 17, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
subprocess.check_call([sys.executable, '-c', script])
subprocess.run([sys.executable, '-c', script], check=True)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@graingert

graingert commented Jul 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

@graingert may I ask for a review pls?

Just doing a eyeball pass at the moment - will do a proper review later. My gut feeling is that I think it's a bit complicated using a thread pool executor and a queue and a pipe and an event so I'll see if I can have a ponder on making this simpler - eg just using threading.Barrier
fantix reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
graingert
graingert reviewed Jul 17, 2022
View reviewed changes
Comment thread tests/test_process.py
import asyncio
import uvloop

uvloop.install()

@graingert graingert Jul 17, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

When the 3.11 test suite lands it might be worth conditionally using the modern asyncio.Runner(loop_factory=uvloop.new_event_loop)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@graingert

graingert commented Jul 19, 2022

Copy link
Copy Markdown
Contributor

@graingert may I ask for a review pls?

Just doing a eyeball pass at the moment - will do a proper review later. My gut feeling is that I think it's a bit complicated using a thread pool executor and a queue and a pipe and an event so I'll see if I can have a ponder on making this simpler - eg just using threading.Barrier

ok I think I've managed to simplify this a lot: https://github.com/MagicStack/uvloop/pull/481/files#r924838769
fantix reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fantix

fantix commented Sep 2, 2022

Copy link
Copy Markdown
Member Author

Close in favor of #481
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@fantix fantix closed this Sep 2, 2022
@fantix
fantix deleted the preexec-fn-double-close branch September 2, 2022 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@graingert graingert graingert left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@fantix @graingert
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.