Skip to content

Don't hold on to channels after they've been closed #1577

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

Closed
tkluck-janestreet wants to merge 1 commit into from
Closed

Don't hold on to channels after they've been closed #1577

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
162 changes: 100 additions & 62 deletions runtime/io.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,77 @@ function caml_sys_open (name, flags, _perms) {

// ocaml Channels

//Provides: caml_ml_set_channel_name
//Requires: caml_ml_channels
function caml_ml_set_channel_name(chanid, name) {
var chan = caml_ml_channels[chanid];
chan.name = name;
return 0;
}
/* The [ChanId] class and the [caml_ml_channels] object are for backwards compatibility
towards ppx_expect. The [caml_ml_channels] used to map from an integer id to a
channel descriptor, but that leaks memory. (In particular, it leaks the buffer
which can be quite large.) We now replaced the integer id by a [ChanId] object,
which, after garbage collection, means we can release its resources.

So the [channel] object can be garbage collected like anything else. The only
small hiccup is that ppx_expect wants to be able to "shadow"/"override"
channels by manipulating the [caml_ml_channels] array. Fortunately, after the
channel is closed we don't need to allow for that anymore. So we keep channels in
the global object until then.

A future version could provide an actual shadowing API for ppx_expect, and get rid
of the last remaining potential memory leak (a channel being discarded before
closing it) entirely.
*/

//Provides: caml_ml_channels
var caml_ml_channels = new Array();
var caml_ml_channels = new Object();
//Provides: caml_ml_open_out_channels
var caml_ml_open_out_channels = new globalThis.Set();

//Provides: ChanId
//Requires: caml_ml_channels, caml_ml_open_out_channels
var ChanId = class {
constructor(channel) {
this.channel = channel;
caml_ml_channels[this] = this;
if(channel.out) caml_ml_open_out_channels.add(this);
}
close() {
delete caml_ml_channels[this];
caml_ml_open_out_channels.delete(this);
}
toString() {
return `[channel for fd=${this.channel.fd}]`
}
get() {
var override = caml_ml_channels[this];
if(override !== undefined) {
return override.channel;
} else {
return this.channel;
}
}
get offset() {
return this.get().offset;
}
};

//Provides: caml_ml_out_channels_list
//Requires: caml_ml_channels
//Requires: caml_ml_open_out_channels
function caml_ml_out_channels_list () {
var l = 0;
for(var c = 0; c < caml_ml_channels.length; c++){
if(caml_ml_channels[c] && caml_ml_channels[c].opened && caml_ml_channels[c].out)
l=[0,caml_ml_channels[c].fd,l];
for(var chanid of caml_ml_open_out_channels) {
l=[0,chanid,l];
}
return l;
}


//Provides: caml_ml_set_channel_name
//Requires: caml_ml_channels
function caml_ml_set_channel_name(chanid, name) {
var chan = chanid.get();
chan.name = name;
return 0;
}

//Provides: caml_ml_open_descriptor_out
//Requires: caml_ml_channels, caml_sys_fds
//Requires: caml_sys_fds, ChanId
//Requires: caml_raise_sys_error
//Requires: caml_sys_open
function caml_ml_open_descriptor_out (fd) {
Expand All @@ -128,12 +174,12 @@ function caml_ml_open_descriptor_out (fd) {
buffer:new Uint8Array(65536),
buffered:buffered
};
caml_ml_channels[channel.fd]=channel;
return channel.fd;
var chanid = new ChanId(channel);
return chanid;
}

//Provides: caml_ml_open_descriptor_in
//Requires: caml_ml_channels, caml_sys_fds
//Requires: caml_sys_fds, ChanId
//Requires: caml_raise_sys_error
//Requires: caml_sys_open
function caml_ml_open_descriptor_in (fd) {
Expand All @@ -151,11 +197,10 @@ function caml_ml_open_descriptor_in (fd) {
buffer:new Uint8Array(65536),
refill:refill
};
caml_ml_channels[channel.fd]=channel;
return channel.fd;
var chanid = new ChanId(channel);
return chanid;
}


//Provides: caml_ml_open_descriptor_in_with_flags
//Requires: caml_ml_open_descriptor_in
//Version: >= 5.1
Expand All @@ -171,68 +216,64 @@ function caml_ml_open_descriptor_out_with_flags(fd, flags){
}

//Provides: caml_channel_descriptor
//Requires: caml_ml_channels
//Alias: win_filedescr_of_channel
function caml_channel_descriptor(chanid){
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return chan.fd;
}

//Provides: caml_ml_set_binary_mode
//Requires: caml_ml_channels
function caml_ml_set_binary_mode(chanid,mode){
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
chan.file.flags.text = !mode
chan.file.flags.binary = mode
return 0;
}

//Provides: caml_ml_is_binary_mode
//Requires: caml_ml_channels
//Version: >= 5.2
function caml_ml_is_binary_mode(chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return chan.file.flags.binary
}

//Input from in_channel

//Provides: caml_ml_close_channel
//Requires: caml_ml_flush, caml_ml_channels
//Requires: caml_ml_flush
//Requires: caml_sys_close
function caml_ml_close_channel (chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
if (!chan.opened) { return 0; }
chan.opened = false;
caml_sys_close(chan.fd)
caml_sys_close(chan.fd);
chanid.close();
Copy link
Member

@hhugo hhugo Mar 3, 2024

Choose a reason for hiding this comment

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

What happen if you close an "overridden" channel twice ?
Don't you end up closing both the override and the original channel ?

Copy link
Author

@tkluck-janestreet tkluck-janestreet Mar 4, 2024

Choose a reason for hiding this comment

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

Good catch, and yes that's what would happen. For example, if you close stderr when you're in the middle of executing an expect test, then the first call will close the temporary output channel instead. Any second call would close stderr itself.

We're stuck with the (non garbage collectible) string keys here, so this is not trivial to fix.

This override logic is only for ppx_expect, and arguably it's pretty poorly defined what closing the channel is supposed to to. But let me see if we can improve the compatibility even more.

return 0;
}

//Provides: caml_ml_channel_size
//Requires: caml_ml_channels
function caml_ml_channel_size(chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return chan.file.length();
}

//Provides: caml_ml_channel_size_64
//Requires: caml_int64_of_float,caml_ml_channels
//Requires: caml_int64_of_float
function caml_ml_channel_size_64(chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return caml_int64_of_float(chan.file.length ());
}

//Provides: caml_ml_set_channel_output
//Requires: caml_ml_channels
function caml_ml_set_channel_output(chanid,f) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
chan.output = (function (s) {f(s)});
return 0;
}

//Provides: caml_ml_set_channel_refill
//Requires: caml_ml_channels
function caml_ml_set_channel_refill(chanid,f) {
caml_ml_channels[chanid].refill = f;
chanid.get().refill = f;
return 0;
}

Expand Down Expand Up @@ -279,9 +320,9 @@ function caml_ml_input_bigarray (chanid, b, i, l) {
}

//Provides: caml_ml_input_block
//Requires: caml_refill, caml_ml_channels
//Requires: caml_refill
function caml_ml_input_block (chanid, ba, i, l) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
var n = l;
var avail = chan.buffer_max - chan.buffer_curr;
if(l <= avail) {
Expand All @@ -305,11 +346,11 @@ function caml_ml_input_block (chanid, ba, i, l) {
}

//Provides: caml_input_value
//Requires: caml_marshal_data_size, caml_input_value_from_bytes, caml_create_bytes, caml_ml_channels, caml_bytes_of_array
//Requires: caml_marshal_data_size, caml_input_value_from_bytes, caml_create_bytes, caml_bytes_of_array
//Requires: caml_refill, caml_failwith, caml_raise_end_of_file
//Requires: caml_marshal_header_size
function caml_input_value (chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
var header = new Uint8Array(caml_marshal_header_size);
function block(buffer, offset, n) {
var r = 0;
Expand Down Expand Up @@ -352,9 +393,9 @@ function caml_input_value_to_outside_heap(c) {

//Provides: caml_ml_input_char
//Requires: caml_raise_end_of_file, caml_array_bound_error
//Requires: caml_ml_channels, caml_refill
//Requires: caml_refill
function caml_ml_input_char (chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
if(chan.buffer_curr >= chan.buffer_max){
chan.buffer_curr = 0;
chan.buffer_max = 0;
Expand All @@ -369,9 +410,8 @@ function caml_ml_input_char (chanid) {

//Provides: caml_ml_input_int
//Requires: caml_raise_end_of_file
//Requires: caml_ml_input_char, caml_ml_channels
//Requires: caml_ml_input_char
function caml_ml_input_int (chanid) {
var chan = caml_ml_channels[chanid];
var res = 0;
for(var i = 0; i < 4; i++){
res = (res << 8) + caml_ml_input_char(chanid) | 0;
Expand All @@ -380,9 +420,9 @@ function caml_ml_input_int (chanid) {
}

//Provides: caml_seek_in
//Requires: caml_raise_sys_error, caml_ml_channels
//Requires: caml_raise_sys_error
function caml_seek_in(chanid, pos) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
if (chan.refill != null) caml_raise_sys_error("Illegal seek");
if(pos >= chan.offset - chan.buffer_max
&& pos <= chan.offset
Expand Down Expand Up @@ -410,9 +450,8 @@ function caml_ml_seek_in_64(chanid,pos){
}

//Provides: caml_pos_in
//Requires: caml_ml_channels
function caml_pos_in(chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return chan.offset - (chan.buffer_max - chan.buffer_curr) | 0;
}

Expand All @@ -430,9 +469,9 @@ function caml_ml_pos_in_64(chanid) {

//Provides: caml_ml_input_scan_line
//Requires: caml_array_bound_error
//Requires: caml_ml_channels, caml_refill
//Requires: caml_refill
function caml_ml_input_scan_line(chanid){
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
var p = chan.buffer_curr;
do {
if(p >= chan.buffer_max) {
Expand All @@ -456,10 +495,10 @@ function caml_ml_input_scan_line(chanid){
}

//Provides: caml_ml_flush
//Requires: caml_raise_sys_error, caml_ml_channels
//Requires: caml_raise_sys_error
//Requires: caml_subarray_to_jsbytes
function caml_ml_flush (chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
if(! chan.opened) caml_raise_sys_error("Cannot flush a closed channel");
if(!chan.buffer || chan.buffer_curr == 0) return 0;
if(chan.output) {
Expand All @@ -476,9 +515,9 @@ function caml_ml_flush (chanid) {

//Provides: caml_ml_output_ta
//Requires: caml_ml_flush,caml_ml_bytes_length
//Requires: caml_raise_sys_error, caml_ml_channels
//Requires: caml_raise_sys_error
function caml_ml_output_ta(chanid,buffer,offset,len) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
if(! chan.opened) caml_raise_sys_error("Cannot output to a closed channel");
buffer = buffer.subarray(offset, offset + len);
if(chan.buffer_curr + buffer.length > chan.buffer.length) {
Expand Down Expand Up @@ -560,10 +599,10 @@ function caml_output_value (chanid,v,flags) {


//Provides: caml_seek_out
//Requires: caml_ml_channels, caml_ml_flush
//Requires: caml_ml_flush
function caml_seek_out(chanid, pos){
caml_ml_flush(chanid);
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
chan.offset = pos;
return 0;
}
Expand All @@ -581,9 +620,9 @@ function caml_ml_seek_out_64(chanid,pos){
}

//Provides: caml_pos_out
//Requires: caml_ml_channels, caml_ml_flush
//Requires: caml_ml_flush
function caml_pos_out(chanid) {
var chan = caml_ml_channels[chanid];
var chan = chanid.get();
return chan.offset + chan.buffer_curr
}

Expand All @@ -610,15 +649,14 @@ function caml_ml_output_int (chanid,i) {
}

//Provides: caml_ml_is_buffered
//Requires: caml_ml_channels
function caml_ml_is_buffered(chanid) {
return caml_ml_channels[chanid].buffered ? 1 : 0
return chanid.get().buffered ? 1 : 0
}

//Provides: caml_ml_set_buffered
//Requires: caml_ml_channels, caml_ml_flush
//Requires: caml_ml_flush
function caml_ml_set_buffered(chanid,v) {
caml_ml_channels[chanid].buffered = v;
chanid.get().buffered = v;
if(!v) caml_ml_flush(chanid);
return 0
}