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.

Sign up for GitHub

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

Jump to bottom

BUG: Don't close stream passed to PdfWriter.write() #2909

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

BUG: Don't close stream passed to PdfWriter.write() #2909

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1d9a52c
BUG: Don't close stream passed to PdfWriter.write()
alexaryn Oct 18, 2024
5568450
mypy
alexaryn Oct 18, 2024
a905bac
Improvements based on code review comments.
alexaryn Oct 21, 2024
dafbafc
Make test work with Python 3.8.
alexaryn Oct 21, 2024
3d6b7dc
Use tmp_path to avoid needing to unlink.
alexaryn Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions pypdf/_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,6 @@ def _get_clone_from(
# to prevent overwriting
self.temp_fileobj = fileobj
self.fileobj = ""
self.with_as_usage = False
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not found of removing with_as_usage attribute. it may be usefull to know that the the object has been created for a context manager.

Copy link
Author

Choose a reason for hiding this comment

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

It's never used. Keeping it around would mislead someone reading the code that it matters in some way. It's dead code but easy to revive if a need arises.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that having a dead property/attribute is not good, but it is a public interface which tends to need a proper deprecation process.

# The root of our page tree node.
pages = DictionaryObject()
pages.update(
Expand Down Expand Up @@ -356,7 +355,6 @@ def __enter__(self) -> "PdfWriter":
"""Store that writer is initialized by 'with'."""
t = self.temp_fileobj
self.__init__() # type: ignore
self.with_as_usage = True
self.fileobj = t # type: ignore
return self

Expand All @@ -369,6 +367,9 @@ def __exit__(
"""Write data to the fileobj."""
if self.fileobj:
self.write(self.fileobj)
close_attr = getattr(self.fileobj, "close", None)
if callable(close_attr):
self.fileobj.close() # type: ignore[attr-defined]
stefan6419846 marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

looking @MasterOdin's post

the stream closure should be done by the caller, not here, no ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO it should be closed on exit to avoid leaking resources - unless I misunderstood the existing discussions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that if a stream is opened by the PdfWriter(eg. if a path is provided)the stream should be closed but if it is the stream (opened before the context of the PdfWriter) it should be closed out of the 'with' section. As written it is always closed at the closure.

Copy link
Author

@alexaryn alexaryn Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm thinking this code isn't needed at all. The only way PdfWriter stores a closable object in self.fileobj is via a with-statement invoking __init__() passing in the object. I note that sometimes self.fileobj is readable and used for cloning and sometimes it's writable and used for output, which is confusing. Also, I think there's a bug in this line: if isinstance(fileobj, (IO, BytesIO)): in that IO is from the typing module and no real objects will inherit from it. I suspect it was meant to be more like IOBase, but I'm not sure. Finally, the fileobj.seek(-1, 2) confuses me; why go one byte back from the end? If the file's empty it'll raise OSError.

In any case, the only file I see being created is in PdfWriter.write() where it's a local variable and it gets closed.


def _repr_mimebundle_(
self,
Expand Down Expand Up @@ -1388,13 +1389,14 @@ def write(self, stream: Union[Path, StrByteType]) -> Tuple[bool, IO[Any]]:

if isinstance(stream, (str, Path)):
stream = FileIO(stream, "wb")
self.with_as_usage = True #
my_file = True

self.write_stream(stream)

if self.with_as_usage:
if my_file:
stream.close()
else:
stream.flush()

return my_file, stream

Expand Down
11 changes: 11 additions & 0 deletions tests/test_writer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2480,3 +2480,14 @@ def test_append_pdf_with_dest_without_page(caplog):
writer.append(reader)
assert "/__WKANCHOR_8" not in writer.named_destinations
assert len(writer.named_destinations) == 3


def test_stream_not_closed():
"""Tests for #2905"""
src = RESOURCE_ROOT / "pdflatex-outline.pdf"
with NamedTemporaryFile() as tmp:
with PdfReader(src) as reader, PdfWriter() as writer:
for i in range(4):
writer.add_page(reader.pages[i])
writer.write(tmp)
assert not tmp.file.closed
Copy link
Collaborator

Choose a reason for hiding this comment

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

it might be great to also have a test for where the automatic write at the closure of the context will be done

Copy link
Author

Choose a reason for hiding this comment

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

I think I managed to add a test for this. It was a bit confusing because of the double-construct that happens in __enter__(). I spent a fair amount of time trying to understand the clone_from logic before I realized that everything from the first __init__() is thrown away except for temp_fileobj.

Loading