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

gh-108308: Replace PyDict_GetItem() with PyDict_GetItemRef() #108309

Merged
merged 3 commits into from Aug 23, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 22, 2023

Replace PyDict_GetItem() calls with PyDict_GetItemRef() to handle errors.

@vstinner vstinner changed the title gh->108308: Replace PyDict_GetItem() with PyDict_GetItemRef() gh-108308: Replace PyDict_GetItem() with PyDict_GetItemRef() Aug 22, 2023
@vstinner
Copy link
Member Author

cc @serhiy-storchaka

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

I actually tried to make the compiler code clearer. But it requires so much work that I keep putting it off after another attempt.

Python/assemble.c Outdated Show resolved Hide resolved
Python/compile.c Outdated Show resolved Hide resolved
Python/flowgraph.c Outdated Show resolved Hide resolved
Python/pylifecycle.c Outdated Show resolved Hide resolved
Replace PyDict_GetItem() calls with PyDict_GetItemRef() to handle
errors.

pycore_init_builtins() now checks for _PyType_Lookup() failure.
@vstinner
Copy link
Member Author

@serhiy-storchaka: I addressed your review. Would you mind to review the updated PR?

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@vstinner vstinner merged commit f5559f3 into python:main Aug 23, 2023
22 checks passed
@bedevere-bot
Copy link

There's a new commit after the PR has been approved.

@serhiy-storchaka: please review the changes made to this pull request.

@vstinner vstinner deleted the use_dict_getitemref branch August 23, 2023 15:40
@vstinner
Copy link
Member Author

Well, writing correct code requires to write more code. IMO it's worth it :-)

@vstinner
Copy link
Member Author

Merged, thanks for the review Serhiy!

@bedevere-bot
Copy link

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

Hi! The buildbot AMD64 RHEL8 3.x has failed when building commit f5559f3.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/185/builds/4792) and take a look at the build logs.
  4. Check if the failure is related to this commit (f5559f3) or if it is a false positive.
  5. 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/185/builds/4792

Failed tests:

  • test.test_asyncio.test_unix_events

Failed subtests:

  • test_fork_signal_handling - test.test_asyncio.test_unix_events.TestFork.test_fork_signal_handling

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

== Tests result: FAILURE then ENV CHANGED ==

432 tests OK.

10 slowest tests:

  • test_gdb: 6 min 2 sec
  • test_concurrent_futures: 2 min 32 sec
  • test_multiprocessing_spawn: 2 min 27 sec
  • test_math: 2 min 3 sec
  • test_multiprocessing_forkserver: 1 min 31 sec
  • test_multiprocessing_fork: 1 min 18 sec
  • test_capi: 58.0 sec
  • test_signal: 57.1 sec
  • test_unparse: 51.8 sec
  • test_tokenize: 51.5 sec

1 test altered the execution environment:
test_ssl

14 tests skipped:
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test_devpoll test_ioctl
test_kqueue test_launcher test_startfile test_tkinter test_ttk
test_winconsoleio test_winreg test_winsound test_wmi
test_zipfile64

1 re-run test:
test.test_asyncio.test_unix_events

Total duration: 7 min 45 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64/build/Lib/unittest/async_case.py", line 90, in _callTestMethod
    if self._callMaybeAsync(method) is not None:
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64/build/Lib/unittest/async_case.py", line 117, in _callMaybeAsync
    return self._asyncioTestContext.run(func, *args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64/build/Lib/test/support/hashlib_helper.py", line 49, in wrapper
    return func_or_class(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/buildbot/buildarea/3.x.cstratak-RHEL8-x86_64/build/Lib/test/test_asyncio/test_unix_events.py", line 1937, in test_fork_signal_handling
    self.assertTrue(child_handled.is_set())
AssertionError: False is not true

@bedevere-bot
Copy link

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

Hi! The buildbot wasm32-emscripten node (pthreads) 3.x has failed when building commit f5559f3.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1050/builds/2826) and take a look at the build logs.
  4. Check if the failure is related to this commit (f5559f3) or if it is a false positive.
  5. 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/1050/builds/2826

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

== Tests result: ENV CHANGED ==

329 tests OK.

10 slowest tests:

  • test_math: 2 min 10 sec
  • test_hashlib: 1 min 31 sec
  • test_unparse: 54.2 sec
  • test_tarfile: 43.2 sec
  • test_tokenize: 37.2 sec
  • test_io: 26.7 sec
  • test_unicodedata: 19.5 sec
  • test_capi: 19.3 sec
  • test_pathlib: 19.1 sec
  • test_fstring: 17.7 sec

1 test altered the execution environment:
test_capi

117 tests skipped:
test.test_asyncio.test_base_events
test.test_asyncio.test_buffered_proto
test.test_asyncio.test_context
test.test_asyncio.test_eager_task_factory
test.test_asyncio.test_events test.test_asyncio.test_futures
test.test_asyncio.test_futures2 test.test_asyncio.test_locks
test.test_asyncio.test_pep492
test.test_asyncio.test_proactor_events
test.test_asyncio.test_protocols test.test_asyncio.test_queues
test.test_asyncio.test_runners
test.test_asyncio.test_selector_events
test.test_asyncio.test_sendfile test.test_asyncio.test_server
test.test_asyncio.test_sock_lowlevel test.test_asyncio.test_ssl
test.test_asyncio.test_sslproto test.test_asyncio.test_streams
test.test_asyncio.test_subprocess
test.test_asyncio.test_taskgroups test.test_asyncio.test_tasks
test.test_asyncio.test_threads test.test_asyncio.test_timeouts
test.test_asyncio.test_transports
test.test_asyncio.test_unix_events test.test_asyncio.test_waitfor
test.test_asyncio.test_windows_events
test.test_asyncio.test_windows_utils test__xxinterpchannels
test__xxsubinterpreters test_asyncgen test_clinic test_cmd_line
test_concurrent_futures test_contextlib_async test_ctypes
test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_doctest
test_docxmlrpc test_dtrace test_embed test_epoll test_faulthandler
test_fcntl test_file_eintr test_fork1 test_ftplib test_gdb
test_generated_cases test_grp test_httplib test_httpservers
test_idle test_imaplib test_interpreters test_ioctl test_kqueue
test_launcher test_lzma test_mmap test_multiprocessing_fork
test_multiprocessing_forkserver test_multiprocessing_main_handling
test_multiprocessing_spawn test_openpty test_pdb
test_perf_profiler test_perfmaps test_poll test_poplib test_pty
test_pwd test_readline test_regrtest test_repl test_resource
test_select test_selectors test_smtplib test_smtpnet test_socket
test_socketserver test_ssl test_stable_abi_ctypes test_startfile
test_subprocess test_sys_settrace test_syslog test_tcl
test_tkinter test_tools test_ttk test_ttk_textonly test_turtle
test_urllib2 test_urllib2_localnet test_urllib2net test_urllibnet
test_venv test_wait3 test_wait4 test_webbrowser test_winconsoleio
test_winreg test_winsound test_wmi test_wsgiref test_xmlrpc
test_xxlimited test_zipfile64 test_zipimport_support test_zoneinfo

Total duration: 21 min 36 sec

Click to see traceback logs
Traceback (most recent call last):
  File "/opt/buildbot/bcannon-wasm/3.x.bcannon-wasm.emscripten-node-pthreads/build/Lib/test/test_capi/test_watchers.py", line 532, in watcher
    raise MyError("testing 123")

@vstinner
Copy link
Member Author

The buildbot wasm32-emscripten node (pthreads) 3.x
1 test altered the execution environment: test_capi

I created #108373 to track this unrelated bug.

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

Successfully merging this pull request may close these issues.

None yet

3 participants