Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
15 changes: 15 additions & 0 deletions integration/run_python_tests.sh
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,19 @@ for file in $(find $SCRIPT_DIR/tests/ -maxdepth 1 -type f -name "*.py"); do
fi
done

# An uncaught exception must exit non-zero, and the exit-time flush of the
# buffered sys.stdout must run before the traceback is printed, so with a
# redirected stdout the script's own output comes first.
file=$SCRIPT_DIR/tests/expected_failures/print_then_raise.py
output=$(timeout 10s $PYTHON_EXECUTABLE $file --gc-frequency $GC_FREQUENCY 2>&1)
if [ $? -eq 0 ]; then
echo $file "... FAILED! (expected a non-zero exit code)"
exit_code=1
elif [ "$(echo "$output" | head -n 1)" != "before-raise" ]; then
echo $file "... FAILED! (script output must precede the traceback, got: ${output})"
exit_code=1
else
echo $file "... PASSED!"
fi

Comment thread
gf712 marked this conversation as resolved.
exit $exit_code
160 changes: 160 additions & 0 deletions integration/tests/buffered_writer.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,160 @@
# BufferedWriter must respect buffer_size like CPython (Modules/_io/bufferedio.c):
# - writes that fit in the buffer's free space are only buffered (no raw write)
# - writes that don't fit first drain the buffer to the raw stream
# - payloads larger than buffer_size bypass the buffer and go straight to raw
# - the remaining tail (<= buffer_size) is buffered again
import _io

PATH = "/tmp/pycpp_buffered_writer_test.bin"


def file_contents():
f = _io.FileIO(PATH, "rb")
data = f.readall()
f.close()
return data


# 1. a small write stays in the buffer until flush
w = _io.BufferedWriter(_io.FileIO(PATH, "wb"))
assert w.write(b"abcd") == 4
assert file_contents() == b"", file_contents()
w.flush()
assert file_contents() == b"abcd", file_contents()

# 2. overflowing the buffer drains it, and an oversized payload bypasses the buffer
w = _io.BufferedWriter(_io.FileIO(PATH, "wb"), 8)
assert w.write(b"abcd") == 4
assert file_contents() == b"", file_contents()
assert w.write(b"efghijklm") == 9
assert file_contents() == b"abcdefghijklm", file_contents()

# 3. a tail smaller than buffer_size stays buffered after the drain
w = _io.BufferedWriter(_io.FileIO(PATH, "wb"), 8)
assert w.write(b"abcdef") == 6
assert w.write(b"ghi") == 3
assert file_contents() == b"abcdef", file_contents()
w.flush()
assert file_contents() == b"abcdefghi", file_contents()

# 4. an exact fit is fully buffered
w = _io.BufferedWriter(_io.FileIO(PATH, "wb"), 8)
assert w.write(b"12345678") == 8
assert file_contents() == b"", file_contents()
w.flush()
assert file_contents() == b"12345678", file_contents()

# 5. buffer_size must be strictly positive
try:
_io.BufferedWriter(_io.FileIO(PATH, "wb"), 0)
assert False, "expected ValueError"
except ValueError:
pass


# 6. raw can be any duck-typed object; a write() that over-reports the byte
# count must raise OSError instead of wrapping the unsigned byte counters
class OverReportingWriter:
def write(self, b):
return 1000


w = _io.BufferedWriter(OverReportingWriter(), 8)
try:
w.write(b"0123456789abcdef")
assert False, "expected OSError"
except OSError:
pass


# 7. same for a negative count
class NegativeWriter:
def write(self, b):
return -1


w = _io.BufferedWriter(NegativeWriter(), 8)
try:
w.write(b"0123456789abcdef")
assert False, "expected OSError"
except OSError:
pass

# 8. a raw write() returning 0 makes no progress; retrying forever would hang,
# so it must raise OSError
class ZeroWriter:
def write(self, b):
return 0


w = _io.BufferedWriter(ZeroWriter(), 8)
try:
w.write(b"0123456789abcdef")
assert False, "expected OSError"
except OSError:
pass


# 9. a raw write() returning None means the stream accepted no data without
# blocking; that is an OSError (BlockingIOError in CPython), not a crash
class NoneWriter:
def write(self, b):
return None


w = _io.BufferedWriter(NoneWriter(), 8)
try:
w.write(b"0123456789abcdef")
assert False, "expected OSError"
except OSError:
pass


# 10. any other non-int return from raw write() is a TypeError, not a crash
class StringWriter:
def write(self, b):
return "16"


w = _io.BufferedWriter(StringWriter(), 8)
try:
w.write(b"0123456789abcdef")
assert False, "expected TypeError"
except TypeError:
pass

# 11. an object created via __new__ without __init__ has no raw stream; every
# I/O method must raise ValueError instead of dereferencing it
uninitialized = _io.BufferedWriter.__new__(_io.BufferedWriter)
for method in (
uninitialized.isatty,
uninitialized.flush,
lambda: uninitialized.write(b"x"),
):
try:
method()
assert False, "expected ValueError"
except ValueError:
pass

# 12. the oversized path calls back into Python once per chunk; a raw whose
# write() reallocates the source object's storage must not leave the write
# loop walking freed memory
ba = bytearray(b"A" * 100)
chunks = []


class MutatingWriter:
def write(self, b):
# reallocates ba's backing storage mid-write
ba[0:1] = b"B" * 200000
chunks.append(1)
return 1


w = _io.BufferedWriter(MutatingWriter(), 8)
assert w.write(ba) == 100
# 100 bytes, one accepted per call, until the 8 byte tail is buffered instead
assert len(chunks) == 92, len(chunks)

print("buffered_writer: ok")
6 changes: 6 additions & 0 deletions integration/tests/expected_failures/print_then_raise.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Buffered sys.stdout must be flushed by the interpreter's exit callbacks
# *before* the uncaught-exception traceback is printed, so with a redirected
# stdout the script's own output comes first. run_python_tests.sh checks the
# output ordering and that the exit code is non-zero.
print("before-raise")
raise RuntimeError("boom")
7 changes: 6 additions & 1 deletion src/executable/bytecode/BytecodeProgram.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#include "Bytecode.hpp"
#include "executable/Function.hpp"
#include "executable/Mangler.hpp"
#include "interpreter/InterpreterSession.hpp"
#include "interpreter/Interpreter.hpp"
#include "runtime/PyCode.hpp"
#include "runtime/PyFrame.hpp"
#include "runtime/PyFunction.hpp"
Expand Down Expand Up @@ -136,6 +136,11 @@ int BytecodeProgram::execute(VirtualMachine *vm)

auto result = m_main_function->function()->call(*vm, interpreter);

{
ScopedStack scoped_stack{ vm->push_frame(1, 0, 0) };
[[maybe_unused]] auto final_result = interpreter.finalise();
}
Comment thread
gf712 marked this conversation as resolved.

if (result.is_err()) {
// The exception propagated all the way out; the eval loop already popped it
// off the (now-clean) exception stack as it unwound, so use the result value
Expand Down
62 changes: 57 additions & 5 deletions src/interpreter/Interpreter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "runtime/Import.hpp"
#include "runtime/KeyError.hpp"
#include "runtime/NameError.hpp"
#include "runtime/PyBool.hpp"
#include "runtime/PyCode.hpp"
#include "runtime/PyDict.hpp"
#include "runtime/PyFrame.hpp"
Expand All @@ -26,6 +27,7 @@

#include <cstdio>
#include <filesystem>
#include <ranges>
#include <unistd.h>

namespace fs = std::filesystem;
Expand Down Expand Up @@ -209,8 +211,13 @@ void Interpreter::internal_setup(const std::string &name,
return buffered_writer->call(PyTuple::create(stdout).unwrap(), nullptr);
})
.and_then([text_io_wrapper](PyObject *stdout_buffer_writer) {
PyObject *line_buffering =
::isatty(STDOUT_FILENO) == 1 ? py_true() : py_false();
return text_io_wrapper->call(
PyTuple::create(stdout_buffer_writer).unwrap(), nullptr);
PyTuple::create(
stdout_buffer_writer, py_none(), py_none(), py_none(), line_buffering)
.unwrap(),
nullptr);
});
ASSERT(py_stdout.is_ok());
auto py_stderr =
Expand All @@ -220,13 +227,30 @@ void Interpreter::internal_setup(const std::string &name,
return buffered_writer->call(PyTuple::create(stderr).unwrap(), nullptr);
})
.and_then([text_io_wrapper](PyObject *stderr_buffer_writer) {
auto *line_buffering = py_true();
return text_io_wrapper->call(
PyTuple::create(stderr_buffer_writer).unwrap(), nullptr);
PyTuple::create(
stderr_buffer_writer, py_none(), py_none(), py_none(), line_buffering)
.unwrap(),
nullptr);
});
ASSERT(py_stderr.is_ok());
sys->add_symbol(PyString::create("stdin").unwrap(), py_stdin.unwrap());
sys->add_symbol(PyString::create("stdout").unwrap(), py_stdout.unwrap());
sys->add_symbol(PyString::create("stderr").unwrap(), py_stderr.unwrap());

for (auto [name, obj] : std::views::zip(std::array{ "stdin", "stdout", "stderr" },
std::array{ py_stdin.unwrap(), py_stdout.unwrap(), py_stderr.unwrap() })) {
sys->add_symbol(PyString::create(name).unwrap(), obj);
register_callback(PyNativeFunction::create(
std::string{ name } + "_exit",
[obj](PyTuple *, PyDict *) -> PyResult<PyObject *> {
return obj->get_method(PyString::create("flush").unwrap())
.and_then([](PyObject *flush) {
return flush->call(nullptr, nullptr);
});
},
obj)
.unwrap(),
nullptr);
}
}

if (config.requires_importlib) {
Expand Down Expand Up @@ -489,4 +513,32 @@ void Interpreter::visit_graph(::Cell::Visitor &visitor)
if (m_codec_error_registry) visitor.visit(*m_codec_error_registry);
if (m_codec_search_path) visitor.visit(*m_codec_search_path);
if (m_codec_search_path_cache) visitor.visit(*m_codec_search_path_cache);

for (auto [callback, args] : m_callbacks) {
if (callback) visitor.visit(*callback);
if (args) visitor.visit(*args);
}
}

void Interpreter::register_callback(PyObject *callback, PyTuple *args)
{
m_callbacks.emplace_back(callback, args);
}

void Interpreter::unregister_callback(PyObject *callback)
{
m_callbacks.erase(
std::ranges::remove(m_callbacks, callback, [](const auto &el) { return std::get<0>(el); })
.begin(),
m_callbacks.end());
}

PyResult<std::monostate> Interpreter::finalise()
{
// TODO: capture exceptions, and raise last one
for (auto [callback, args] : m_callbacks) {
[[maybe_unused]] auto result = callback->call(args, nullptr);
}

return Ok(std::monostate{});
Comment on lines +536 to +543

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

finalise() discards the result of every exit callback ([[maybe_unused]] auto result = ...) and always returns Ok. The caller in BytecodeProgram.cpp also discards finalise()'s return value, so the process exit code depends only on the main function's result. Since this same PR makes print() default to flush=False and ties sys.stdout line-buffering to isatty(), a flush failure during exit (e.g. EPIPE on a closed pipe, ENOSPC) is now silently swallowed instead of surfacing as a Python error — output can be silently truncated while the process still exits 0. (bug)

}
8 changes: 8 additions & 0 deletions src/interpreter/Interpreter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <string>
#include <string_view>
#include <variant>

class BytecodeProgram;

Expand Down Expand Up @@ -34,6 +35,7 @@ class Interpreter
py::PyDict *m_codec_search_path_cache{ nullptr };
std::string m_entry_script;
std::vector<std::string> m_argv;
std::vector<std::tuple<py::PyObject *, py::PyTuple *>> m_callbacks;

public:
struct Config
Expand Down Expand Up @@ -89,6 +91,8 @@ class Interpreter
void setup(std::shared_ptr<BytecodeProgram> &&program);
void setup_main_interpreter(std::shared_ptr<BytecodeProgram> &&program);

py::PyResult<std::monostate> finalise();

const std::string &entry_script() const { return m_entry_script; }
const std::vector<std::string> &argv() const { return m_argv; }

Expand All @@ -107,6 +111,10 @@ class Interpreter
py::PyTuple *args,
py::PyDict *kwargs);


void register_callback(py::PyObject *callback, py::PyTuple *args);
void unregister_callback(py::PyObject *callback);

void visit_graph(::Cell::Visitor &);

private:
Expand Down
2 changes: 1 addition & 1 deletion src/repl/repl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -230,4 +230,4 @@ int main(int argc, char **argv)
// }

return EXIT_SUCCESS;
}
}
4 changes: 1 addition & 3 deletions src/runtime/modules/BuiltinsModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,7 @@ PyResult<PyObject *> print(const PyTuple *args, const PyDict *kwargs, Interprete
// sys.stdout may be None when FILE* stdout isn't connected
if (!file || file == py_none()) { return Ok(py_none()); }
}
// TODO: flush should be false, but for now we keep it as true, since there is no flush at
// interpreter shutdown
bool flush = true;
bool flush = false;
if (kwargs) {
static const Value separator_keyword = String{ "sep" };
static const Value end_keyword = String{ "end" };
Expand Down
37 changes: 37 additions & 0 deletions src/runtime/modules/IOChecks.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
#pragma once

#include "runtime/Value.hpp"
#include "runtime/ValueError.hpp"

#include <variant>

namespace py {

// bool is_initialized() const - required by check_initialized()
// bool is_detached() const - optional; reported separately when the object was
// initialized once and then detached
template<typename Derived> class IOChecks
{
const Derived &derived() const { return static_cast<const Derived &>(*this); }

public:
PyResult<std::monostate> check_initialized() const
{
if (derived().is_initialized()) { return Ok(std::monostate{}); }
if constexpr (requires(const Derived &d) { d.is_detached(); }) {
if (derived().is_detached()) {
return Err(value_error("raw stream has been detached"));
}
}
return Err(value_error("I/O operation on uninitialized object"));
}

// the guard pair an accessor wants: usable means constructed *and* still open.
// Spelling it once keeps the two checks from being chained in the wrong order.
PyResult<std::monostate> check_usable() const
{
return check_initialized().and_then([this](auto) { return derived().check_closed(); });
}
};

}// namespace py
Loading
Loading