Skip to content

Read child stdout pipe by chunks rather than by lines - #214

Merged
zhelih merged 1 commit into
masterfrom
stdout_fix
Feb 18, 2026
Merged

Read child stdout pipe by chunks rather than by lines#214
zhelih merged 1 commit into
masterfrom
stdout_fix

Conversation

@zhelih

@zhelih zhelih commented Feb 8, 2026

Copy link
Copy Markdown
Collaborator

This PR aims to address the long standing issue #162

The previous logic was to read the stdout from the REPL pipe line by line.
The new proposed solution aims to read by chunks, at most 1KB size.

Tested locally with various edge cases directly in the notebook.

Comment thread src/repl/process.ml
| _ -> Lwt.return_unit)

let read_exn ic =
match%lwt Lwt_io.read ~count:1024 ic with

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

impl is ok but choice of 1kb feels strange - why is it better than reading by lines?
can do Lwt_io.read_char and accumulate into buffer which is sent by timer,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

From the issue:

in a nutshell print_string("one\ntwo\nthree"); flush stdout will print only one and two. Reason is, the kernel reads stdout from REPL process line by line.

This PR replaces it reading by "chunks", arbitrary chosen at 1KB limit. If stdout has less than 1KB data, this will be read and sent to jupyter too. Tbh, 1KB only to protect from user mistakes, since I doubt somebody will actually output KBs of text to Jupyter out. We rely on Lwt read to perform heavy lifting which might not be guaranteed by specs, potentially would need to replace with reading byte by byte in the future in-house.

@zhelih
zhelih merged commit 205bb43 into master Feb 18, 2026
7 of 8 checks passed
@zhelih
zhelih deleted the stdout_fix branch February 18, 2026 04:23
@zhelih zhelih mentioned this pull request Feb 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants