Skip to content

Remove unused PyLease; skip flaky httpservers test on macOS - #8821

Merged
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:remove-pylease
Sep 26, 2026
Merged

Remove unused PyLease; skip flaky httpservers test on macOS#8821
youknowone merged 2 commits into
RustPython:mainfrom
youknowone:remove-pylease

Conversation

@youknowone

@youknowone youknowone commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

PyLease was introduced in f075cac to wrap a read guard over the object's type while typ was a PyRwLock<PyTypeRef>, so class() could hand out the type without cloning the Arc. Its last users, class() and lease_class(), went away in #3828 when typ became a PyAtomicRef<PyType>. It has carried #[allow(dead_code)] and a "check if we still need this" TODO since.

Its only field is private and there is no constructor, so it cannot be built inside or outside the crate. This removes it and the now-unused PyRwLockReadGuard import.

Also skips test_httpservers.CGIHTTPServerTestCase.test_large_content_length_truncated on macOS again. It failed macOS CI on this PR. The test sets the handler timeout to 1ms, and that timeout also bounds reading the request line: if the client has not sent the request within 1ms, the server logs "Request timed out" and closes the connection, and the client gets ConnectionResetError or BrokenPipeError. It reproduces locally on macOS about 5 runs in 6 with both RustPython and CPython 3.14.6. The skip was removed in #8759 after Linux-only repeats; Linux and Windows keep running it.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Refactor
    • Made an internal implementation cleanup. No user-facing behavior changes are included in this update.

@youknowone
Remove unused PyLease
cf4a5f3 
PyLease wrapped a read guard over the object's type when `typ` was a
`PyRwLock<PyTypeRef>`. Its last users, `class()` and `lease_class()`,
went away when `typ` became a `PyAtomicRef<PyType>` (RustPython#3828). Its field
is private and it has no constructor, so it cannot be built anywhere.

Assisted-by: Claude Code:claude-opus-5-5
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 2026

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 9ec63478-8b8c-4f1c-9a2f-4fe674b828f9

📥 Commits

Reviewing files that changed from the base of the PR and between 393689d and cf4a5f3.

📒 Files selected for processing (1)
  • crates/vm/src/object/ext.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Walkthrough

The change updates the atomic import in ext.rs and removes PyLease and its associated implementations.

Changes

PyLease removal

Layer / File(s) Summary
Update import and remove PyLease
crates/vm/src/object/ext.rs		 The atomic import now uses crate::common::atomic. PyLease and its into_owned, Borrow, Deref, and Display implementations are removed.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to cf4a5

No repository evidence establishes a compatibility requirement or current consumer for the removed, unconstructible type. No demonstrated issue blocks merging.

Architecture Summary

Architecture risk: 🔵 Low · up to cf4a5

The change affects 1 system.

Changed systems: crates

Architecture concerns
No architecture-level concerns identified.

Review details

Systems and components

  • observed — crates (service) was modified; 1 changed file maps to changed impact.

Before / after behavior

  • observed — Modified behavior in crates/vm/src/object/ext.rs: The atomic imports now come directly from crate::common::atomic; the PyRwLockReadGuard import was removed.
  • observed — Modified behavior in crates/vm/src/object/ext.rs: PyLease and its into_owned, Borrow, Deref, and Display implementations were removed. PyLease previously held a read guard over PyRef<T> and cloned that reference when converted to an owned value.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Title check ⚠️ Warning The title correctly identifies removal of the unused PyLease type, which is the main change. However, it also claims that the pull request skips a flaky httpservers test on macOS, and the provided cha… Remove the unsupported “skip flaky httpservers test on macOS” clause, or include the corresponding test change in the pull request if it is intended and required.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check

Explanation

The title correctly identifies removal of the unused PyLease type, which is the main change. However, it also claims that the pull request skips a flaky httpservers test on macOS, and the provided changes do not include that update.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codspeed

codspeed Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 62 untouched benchmarks
⏩ 4 skipped benchmarks1

Comparing youknowone:remove-pylease (cf4a5f3) with main (393689d)

Open in CodSpeed

Footnotes

  1. 4 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@youknowone
Skip test_large_content_length_truncated on macOS
62300b1 
The test sets the CGI handler timeout to 1ms. That timeout also bounds
reading the request line, so when the client has not sent the request
within 1ms the server logs "Request timed out" and closes the
connection. The client then gets ConnectionResetError or
BrokenPipeError. This reproduces on macOS with both RustPython and
CPython 3.14.6. The skip was removed in RustPython#8759 after Linux-only
repeats.

Assisted-by: Claude Code:claude-opus-5-5
@youknowone youknowone changed the title Remove unused PyLease Remove unused PyLease; skip flaky httpservers test on macOS Sep 26, 2026
@github-actions

github-actions Bot commented Sep 26, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[ ] lib: cpython/Lib/http
[ ] test: cpython/Lib/test/test_httplib.py (TODO: 1)
[x] test: cpython/Lib/test/test_http_cookiejar.py
[x] test: cpython/Lib/test/test_http_cookies.py (TODO: 1)
[ ] test: cpython/Lib/test/test_httpservers.py (TODO: 1)

dependencies:

  • http (native: binascii, collections.abc, email.message, email.parser, email.utils, errno, http.client, itertools, pwd, select, sys, time, urllib.parse, urllib.request)
    • io (native: _io, _thread, errno, msvcrt, sys)
    • logging (native: atexit, collections.abc, email.message, email.utils, errno, http.client, logging.handlers, multiprocessing.queues, select, sys, time, urllib.parse, win32evtlog, win32evtlogutil)
    • warnings (native: _contextvars, _thread, _warnings, builtins, sys)
    • argparse, base64, calendar, contextlib, copy, datetime, enum, html, mimetypes, os, posixpath, re, shutil, socket, socketserver, ssl, string, subprocess, threading, traceback, types

dependent tests: (51 tests)

  • http: test_docxmlrpc test_genericalias test_hashlib test_http_cookiejar test_http_cookies test_httplib test_httpservers test_logging test_robotparser test_ssl test_ucn test_unicodedata test_urllib test_urllib2 test_urllib2_localnet test_wsgiref test_xml_dom_xmlbuilder test_xmlrpc
    • logging.handlers: test_concurrent_futures test_pkgutil
    • pydoc: test_enum test_pydoc
      • pdb: test_pdb
    • urllib.request: test_pathlib test_sax test_site test_urllib2net test_urllibnet
      • pathlib: test_ast test_dbm_sqlite3 test_ensurepip test_importlib test_json test_launcher test_os test_pathlib test_peg_generator test_pyrepl test_runpy test_tarfile test_tempfile test_tomllib test_tools test_traceback test_unparse test_venv test_winapi test_zipapp test_zipfile test_zoneinfo test_zstd

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@youknowone
youknowone merged commit 7f314dd into RustPython:main Sep 26, 2026
21 checks passed
@youknowone
youknowone deleted the remove-pylease branch September 26, 2026 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@youknowone