GeezerVison

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

GeezerVison is showing github.com — gh-110038: KqueueSelector must count all read/write events by sorcio · Pull Request #110039 · python/cpython

Conversation

sorcio commented Sep 28, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

With this change, each registered read or write filter is counted separately, even if it shares a fd.

As a side effect, a call to KqueueSelector.select() might return a separate entry with the same key, one with EVENT_READ and the other with EVENT_WRITE (and never both). This was already the case before, but it might become more evident now.

Issue: KqueueSelector miscounts max events #110038

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio added 2 commits

September 28, 2023 18:45


          pythongh-110038: KqueueSelector must count all read/write events

44980b7


          news entry

c32a68a

bedevere-app Bot added the awaiting review label

Sep 28, 2023

bedevere-app Bot mentioned this pull request

Sep 28, 2023

KqueueSelector miscounts max events #110038

Closed


          update the counter only after the syscall succeeded

68a1c2d

vstinner reviewed

Sep 28, 2023

View reviewed changes

Comment thread

Lib/test/test_selectors.py

		result = s.select()
		# We get the read and write either in the same result entry or in two
		# distinct entries with the same key.
		self.assertLessEqual(len(result), 2)

vstinner Sep 28, 2023

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 suggest to move this check at the end, and make sure that len == 2.

Sorry, something went wrong.

Uh oh!

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

All reactions

sorcio Sep 28, 2023

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.

We might get 1 or 2 items, depending on the implementation. I think it's only 2 for KqueueSelector (which is not ideal, but see note in the PR).

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner Sep 28, 2023

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.

Oh, it's possible that a single entry contains both events, I see! Ok.

Sorry, something went wrong.

Uh oh!

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

All reactions

vstinner added needs backport to 3.11

only security fixes

needs backport to 3.12 only security fixes labels

Sep 28, 2023

vstinner approved these changes

Sep 28, 2023

View reviewed changes

vstinner left a comment

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.

LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-app Bot added awaiting core review and removed awaiting review labels

Sep 28, 2023

vstinner enabled auto-merge (squash)

September 28, 2023 17:19

vstinner commented Sep 28, 2023

Copy link

Copy Markdown

Member

Thanks for the test!

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner merged commit b14f0ab into python:main

Sep 28, 2023

miss-islington commented Sep 28, 2023

Copy link

Copy Markdown

Contributor

Thanks @sorcio for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the awaiting core review label

Sep 28, 2023

miss-islington commented Sep 28, 2023

Copy link

Copy Markdown

Contributor

Sorry, @sorcio and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b14f0ab51cb4851b25935279617e388456dcf716 3.12

All reactions

Sorry, something went wrong.

Uh oh!

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

miss-islington assigned vstinner

Sep 28, 2023

miss-islington commented Sep 28, 2023

Copy link

Copy Markdown

Contributor

Sorry, @sorcio and @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker b14f0ab51cb4851b25935279617e388456dcf716 3.11

All reactions

Sorry, something went wrong.

Uh oh!

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

sorcio added a commit to sorcio/cpython that referenced this pull request

Sep 28, 2023


          [3.12] pythongh-110038: KqueueSelector must count all read/write even…

5012cf7

…ts (pythonGH-110039).

(cherry picked from commit b14f0ab)

Co-authored-by: Davide Rizzo <[email protected]>

bedevere-app Bot commented Sep 28, 2023

Copy link

Copy Markdown

GH-110043 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.12 only security fixes label

Sep 28, 2023

bedevere-app Bot commented Sep 28, 2023

Copy link

Copy Markdown

GH-110044 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-app Bot removed the needs backport to 3.11 only security fixes label

Sep 28, 2023

sorcio added a commit to sorcio/cpython that referenced this pull request

Sep 28, 2023


          [3.11] pythongh-110038: KqueueSelector must count all read/write even…

01b23d0

…ts (pythonGH-110039).

(cherry picked from commit b14f0ab)

Co-authored-by: Davide Rizzo <[email protected]>

bedevere-bot commented Sep 28, 2023

Copy link

Copy Markdown

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL8 FIPS Only Blake2 Builtin Hash 3.x has failed when building commit b14f0ab.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/469/builds/6126) and take a look at the build logs.
Check if the failure is related to this commit (b14f0ab) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/469/builds/6126

Failed tests:

test.test_asyncio.test_waitfor

Failed subtests:

test_wait_for_timeout_less_then_0_or_0 - test.test_asyncio.test_waitfor.AsyncioWaitForTest.test_wait_for_timeout_less_then_0_or_0

Summary of the results of the build (if available):

Click to see traceback logs

Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-fips-x86_64.no-builtin-hashes-except-blake2/build/Lib/test/test_asyncio/test_waitfor.py", line 124, in test_wait_for_timeout_less_then_0_or_0
    self.assertLess(t1 - t0, 0.1)
AssertionError: 0.13320737797766924 not less than 0.1

All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner pushed a commit that referenced this pull request

Sep 28, 2023


          [3.11] gh-110038: KqueueSelector must count all read/write events (GH…

d6e8001

…-110039) (#110044)

[3.11] gh-110038: KqueueSelector must count all read/write events (GH-110039).
(cherry picked from commit b14f0ab)

csm10495 pushed a commit to csm10495/cpython that referenced this pull request

Sep 29, 2023


          pythongh-110038: KqueueSelector must count all read/write events (pyt…

eb292de

…hon#110039)

Yhg1s pushed a commit that referenced this pull request

Oct 2, 2023


          [3.12] gh-110038: KqueueSelector must count all read/write events (GH…

c794103

…-110039) (#110043)

[3.12] gh-110038: KqueueSelector must count all read/write events (GH-110039).
(cherry picked from commit b14f0ab)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-110038: KqueueSelector must count all read/write events (pyt…

4b73ee7

…hon#110039)

Reviewers

vstinner

vstinner approved these changes

Uh oh!

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

Assignees

vstinner

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.

4 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 01402520 — 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

sorcio commented Sep 28, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

sorcio Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 28, 2023

Uh oh!

miss-islington commented Sep 28, 2023

Uh oh!

miss-islington commented Sep 28, 2023

Uh oh!

miss-islington commented Sep 28, 2023

Uh oh!

bedevere-app Bot commented Sep 28, 2023

Uh oh!

bedevere-app Bot commented Sep 28, 2023

Uh oh!

bedevere-bot commented Sep 28, 2023

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sorcio commented Sep 28, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

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