GeezerVison

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

GeezerVison is showing github.com — use a stack of self._fds_to_close to prevent double closes by graingert · Pull Request #481 · MagicStack/uvloop

Conversation

graingert commented Jul 19, 2022 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

and make tests easier to write because the close order is deterministic
and in the order that opens happen in

this should also be a bit faster because list.append is faster
than set.add and we skip a call to os_close(-1) and catching an
OSError exception

Sorry, something went wrong.

Uh oh!

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

All reactions

fantix and others added 3 commits

July 17, 2022 11:22


          Add test for preexec_fn fd double close issue

b1f4f3d


          Update tests/test_process.py

757e39c

Co-authored-by: Thomas Grainger <[email protected]>


          use a stack of self._fds_to_close to prevent double closes

730dd1a

and make tests easier to write because the close order is deterministic
and in the order that opens happen in

this should also be a bit faster because list.append is faster
than set.add and we skip a call to os_close(-1) and catching an
OSError exception

graingert changed the base branch from preexec-fn-double-close to master

July 19, 2022 18:29


          Merge branch 'master' into preexec-fn-double-close

ddeb5e1

graingert commented

Jul 19, 2022

View reviewed changes

Comment thread

tests/test_process.py


		async def test():
		proc = await asyncio.create_subprocess_exec(
		sys.executable, "-c", "pass"

graingert Jul 19, 2022

Copy link

Copy Markdown

Contributor Author

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.

you don't actually need a preexec_fn to trigger the double close it just made it slow enough that the race was more detectable

Sorry, something went wrong.

Uh oh!

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

All reactions

fantix reviewed

Jul 19, 2022

View reviewed changes

Comment thread

uvloop/handles/process.pyx

		pass

		fds_to_close = self._fds_to_close
		self._fds_to_close = None

fantix Jul 19, 2022

Copy link

Copy Markdown

Member

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.

I think self._fds_to_close = None is still needed?

Sorry, something went wrong.

Uh oh!

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

All reactions

graingert Jul 19, 2022

Copy link

Copy Markdown

Contributor Author

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.

Ah yep I see it

Sorry, something went wrong.

Uh oh!

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

All reactions

graingert Jul 19, 2022

Copy link

Copy Markdown

Contributor Author

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.

I moved it up so that _close_after_spawn can't interfere with the errpipe_write fd

Sorry, something went wrong.

Uh oh!

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

All reactions


          Update process.pyx

f4f57f6

graingert commented

Jul 19, 2022

View reviewed changes

Comment thread

uvloop/handles/process.pyx Resolved

Uh oh!

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


          Update uvloop/handles/process.pyx

d5fe279

graingert requested a review from fantix

July 20, 2022 09:19


          DRY os_dup call

graingert commented

Jul 20, 2022

View reviewed changes

Comment thread

uvloop/handles/process.pyx

Comment on lines -503 to +499

		newfd = os_dup(io[1])
		os_set_inheritable(newfd, True)
		self._close_after_spawn(newfd)
		io[2] = newfd
		io[2] = self._file_redirect_stdio(io[1])

graingert Jul 20, 2022

Copy link

Copy Markdown

Contributor Author

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 reviewing all the calls to _close_after_spawn to make sure a set wasn't needed, I found this duplicate code

uvloop/uvloop/handles/process.pyx

Lines 398 to 402 in 5926386

	cdef _file_redirect_stdio(self, int fd):
	fd = os_dup(fd)
	os_set_inheritable(fd, True)
	self._close_after_spawn(fd)
	return fd

Sorry, something went wrong.

Uh oh!

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

fantix reacted with thumbs up emoji

All reactions

👍 1 reaction

graingert closed this

Jul 22, 2022

graingert reopened this

Jul 22, 2022

graingert commented Jul 29, 2022

Copy link

Copy Markdown

Contributor Author

@fantix can I get a review on this please?

All reactions

Sorry, something went wrong.

Uh oh!

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


          Merge branch 'master' into preexec-fn-double-close

aeb5560

graingert commented

Aug 16, 2022

View reviewed changes

Comment thread

uvloop/handles/process.pxd

		bint _restore_signals

		set _fds_to_close
		list _fds_to_close

graingert Aug 16, 2022

Copy link

Copy Markdown

Contributor Author

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.

@fantix I think there's a proper list[int] data type in cython for this but I don't know how to use it

Sorry, something went wrong.

Uh oh!

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

All reactions

fantix approved these changes

Aug 23, 2022

View reviewed changes

fantix merged commit 3214cf6 into MagicStack:master

Aug 23, 2022

graingert deleted the preexec-fn-double-close branch

August 23, 2022 18:48

graingert commented Aug 24, 2022

Copy link

Copy Markdown

Contributor Author

@fantix thanks for the review!

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 mentioned this pull request

Sep 2, 2022

Add test for preexec_fn fd double close issue #479

Closed

fantix added a commit that referenced this pull request

Sep 13, 2022


          uvloop v0.17.0

c4c3f9f

This release adds Python 3.11 support, updates bundled libuv to 1.43.0
and fixes a handful of issues.

Changes
=======

* Expose uv_loop_t pointer for integration with other C-extensions (#310)
  (by @pranavtbhat in b332eb8 for #310)

* Support python 3.11+ (#473)
  (by @zeroday0619 in 8e42921 for #473)

* Expose libuv uv_fs_event functionality (#474)
  (by @jensbjorgensen @fantix in 74d381e for #474)

* Activate debug mode when `-X dev` is used
  (by @jack1142 in 637a77a)

* Expose uv_version() for libuv API compatibility (#491)
  (by @fantix in 089f6cb for #491)

* Fix loop.getaddrinfo() and tests (#495)
  (by @fantix in 598b16f for #495)

* Bump to libuv 1.43.0
  (by @fantix in 94e5e53)

Fixes
=====

* _TransProtPair is no longer defined in asyncio.events
  (by @jensbjorgensen in fae5f7f)

* use a TypeVar for asyncio.BaseProtocol (#478)
  (by @graingert in 3aacb35 for #478)

* Fix segfault in TimerHandle.when() after cleared
  (by @jensbjorgensen in c39afff for #469)

* Avoid self._errpipe_write double close (#466)
  (by @graingert in 72140d7 for #466)

* Fix typo in test (#456)
  (by @kianmeng in 033d52d for #456)

* Fix potential infinite loop (#446)
  (by @kfur in ada43c0 for #446)

* use a stack of self._fds_to_close to prevent double closes (#481)
  (by @graingert in 3214cf6 for #481)

* Fix incorrect main thread id value forking from a thread  (#453)
  (by @horpto @fantix in e7934c8 for #453)

* create_subprocess_exec should treat env={} as empty environment (#439) (#454)
  (by @byllyfish in e04637e for #439)

* Queue write only after processing all buffers (#445)
  (by @jakirkham @fantix in 9c6ecb6 for #445)

* Drop Python 3.6 support for thread ident
  (by @fantix in 9c37930)

* bugfix: write to another transport in resume_writing() fails (#498)
  (by @fantix in d2deffe for #498)

Build
=====

* Upgrade GitHub Actions (#477) (#480)
  (by @cclauss in fcbf422 for #477, 1008694 for #480)

* typo `same as same`
  (by @YoSTEALTH in fedba80)

* setup.py: allow to override extra_compile_args (#443)
  (by @giuliobenetti in a130375 for #443)

* Drop hack in setup.py in finalize_options (492)
  (by @fantix in 2f1bc83 for #492)

* Fix tests invocation on release CI worklow (#489)
  (by @ben9923 in d6a2b59 for #489)

Documentation
=============

* use asyncio.Runner loop_factory on 3.11+ (#472)
  (by @graingert in 31ba48c for #472)

* Fix CI badge in docs, remove remaining Travis CI references from docs
  (by @Nothing4You in c6901a7)

* Fix typo in README
  (by @monosans in 73d7253)

fantix mentioned this pull request

Sep 13, 2022

uvloop v0.17.0 #501

Merged

fantix added a commit that referenced this pull request

Sep 14, 2022


          uvloop v0.17.0

afb3268

This release adds Python 3.11 support, updates bundled libuv to 1.43.0
and fixes a handful of issues.

Changes
=======

* Expose uv_loop_t pointer for integration with other C-extensions (#310)
  (by @pranavtbhat in b332eb8 for #310)

* Support python 3.11+ (#473)
  (by @zeroday0619 in 8e42921 for #473)

* Expose libuv uv_fs_event functionality (#474)
  (by @jensbjorgensen @fantix in 74d381e for #474)

* Activate debug mode when `-X dev` is used
  (by @jack1142 in 637a77a)

* Expose uv_version() for libuv API compatibility (#491)
  (by @fantix in 089f6cb for #491)

* Fix loop.getaddrinfo() and tests (#495)
  (by @fantix in 598b16f for #495)

* Bump to libuv 1.43.0
  (by @fantix in 94e5e53)

Fixes
=====

* _TransProtPair is no longer defined in asyncio.events
  (by @jensbjorgensen in fae5f7f)

* use a TypeVar for asyncio.BaseProtocol (#478)
  (by @graingert in 3aacb35 for #478)

* Fix segfault in TimerHandle.when() after cleared
  (by @jensbjorgensen in c39afff for #469)

* Avoid self._errpipe_write double close (#466)
  (by @graingert in 72140d7 for #466)

* Fix typo in test (#456)
  (by @kianmeng in 033d52d for #456)

* Fix potential infinite loop (#446)
  (by @kfur in ada43c0 for #446)

* use a stack of self._fds_to_close to prevent double closes (#481)
  (by @graingert in 3214cf6 for #481)

* Fix incorrect main thread id value forking from a thread  (#453)
  (by @horpto @fantix in e7934c8 for #453)

* create_subprocess_exec should treat env={} as empty environment (#439) (#454)
  (by @byllyfish in e04637e for #439)

* Queue write only after processing all buffers (#445)
  (by @jakirkham @fantix in 9c6ecb6 for #445)

* Drop Python 3.6 support for thread ident
  (by @fantix in 9c37930)

* bugfix: write to another transport in resume_writing() fails (#498)
  (by @fantix in d2deffe for #498)

Build
=====

* Upgrade GitHub Actions (#477) (#480)
  (by @cclauss in fcbf422 for #477, 1008694 for #480)

* typo `same as same`
  (by @YoSTEALTH in fedba80)

* setup.py: allow to override extra_compile_args (#443)
  (by @giuliobenetti in a130375 for #443)

* Drop hack in setup.py in finalize_options (492)
  (by @fantix in 2f1bc83 for #492)

* Fix tests invocation on release CI worklow (#489)
  (by @ben9923 in d6a2b59 for #489)

Documentation
=============

* use asyncio.Runner loop_factory on 3.11+ (#472)
  (by @graingert in 31ba48c for #472)

* Fix CI badge in docs, remove remaining Travis CI references from docs
  (by @Nothing4You in c6901a7)

* Fix typo in README
  (by @monosans in 73d7253)

Reviewers

fantix

fantix approved these changes

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

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.

GeezerVison

Conversation

graingert commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

graingert Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

fantix Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

graingert Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

graingert Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

graingert Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

graingert commented Jul 29, 2022

Uh oh!

graingert Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

graingert commented Aug 24, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

graingert commented Jul 19, 2022 •

edited

Loading

Uh oh!

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