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.

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

Issue #5258: the patch is incorrect #5340

Closed
vicuna opened this issue Aug 16, 2011 · 7 comments
Closed

Issue #5258: the patch is incorrect #5340

vicuna opened this issue Aug 16, 2011 · 7 comments
Labels

Comments

@vicuna
Copy link

vicuna commented Aug 16, 2011

Original bug ID: 5340
Reporter: @db4
Status: closed (set by @xavierleroy on 2015-12-11T18:04:22Z)
Resolution: not a bug
Priority: normal
Severity: minor
Version: 3.12.1
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @protz @ygrek @alainfrisch

Bug description

(I cannot reopen the issue so I have to create another one)

The original reporter decided that

let () =
let state = ref 0 in
while true do
Printf.printf "Running loop #%d\n%!" !state;
Thread.delay 0.1;
let fd = Unix.openfile "test" [Unix.O_RDONLY] 0o640 in
Printf.printf "Opened file\n%!";

(* This function implicitely creates
 * a C runtime descriptor.
 * Uncommenting it leads to "Too many
 * open file" after (here) 2046 iterations. *)
(* ignore(Unix.in_channel_of_descr fd); *)

Unix.close fd;
Printf.printf "Closed file\n%!";
incr state

done

is a problem, and created a fix. IMHO, there is no bug here; once
you constructed a channel from a file descriptor, the descriptor is owned by
the channel, and you only have to close the latter:

let fd = Unix.openfile "test" [Unix.O_RDONLY] 0o640 in
let ch = Unix.in_channel_of_descr fd) in
close_in ch;

So fixing a non-existent issue created the new one: OCaml debugger under Win32 is broken now (and probably many other libraries that use Unix.in_channel_of_descr/Unix.out_channel_of_descr). It's a pity that an untested patch went to the release so easily (rev 11030 in the OCaml repository).

That said, there still is a real descriptor leak in Unix.fstat:

let fd = Unix.openfile "test" [Unix.O_RDONLY] 0o640 in
ignore(Unix.fstat fd);
Unix.close fd;

Probably the proposed

CRT_fd_val(handle) = fd;

will be OK there.

@vicuna
Copy link
Author

vicuna commented Aug 23, 2011

Comment author: @damiendoligez

is a problem, and created a fix. IMHO, there is no bug here; once

There is at least a discrepancy: under Unix, the file descriptor is shared by
all the channels opened on it. This is what the patch tries to fix.

you constructed a channel from a file descriptor, the descriptor is owned by
the channel, and you only have to close the latter:

There is something I don't understand, here. As far as I can tell, this is still
true after applying the patch.

OCaml debugger under Win32 is broken now

Could you give more details ? Why is it broken ? I'd like to see if we can get
a fix that satisfies everyone.

@vicuna
Copy link
Author

vicuna commented Aug 23, 2011

Comment author: @db4

Now after

let fd = Unix.openfile "test" [Unix.O_RDONLY] 0o640 in
let ch = Unix.in_channel_of_descr fd) in
Unix.close fd;
(...)

(Unix.close fd) also closes the underlying channel's file descriptor, and all subsequent channel I/O operations will fail. It was not the case before. Is it normal behavior under Unix? I assume that's why OCaml debugger could not connect to debugged process. I did not do much testing: just reverted the patch and the debugger started to work again. But I promise to provide more details in the near future.

@vicuna
Copy link
Author

vicuna commented Aug 24, 2011

Comment author: @xavierleroy

Hello Dmitry,

Yes, what you describe is normal behavior under Unix: the in_channel "ch" shares the same OS file descriptor as the Unix.filedescr "fd".

I can't find documentation on _open_ofshandle() that says whether it duplicates the given HANDLE or just uses it as is. The former would explain some of your observations, maybe not all.

Thanks for any light you can shed here.

@vicuna
Copy link
Author

vicuna commented Aug 24, 2011

Comment author: ripoche

I just wanted to point out that the patch from bug 5258 seems to fix a rather annoying OMake issue.

Without the patch, OMake freezes randomly on Windows in the middle of builds. You then have to restart it manually.

I have no absolute certainty here, but I couldn't get it to freeze with the patch. Previously, every other build triggered the issue.

I've been using this version for just a couple of days, it is too early for conclusions.

For reference, that's OMake 0.9.8.6 (RC1) built with OCaml 3.11.2 + patch of bug 5258.

@vicuna
Copy link
Author

vicuna commented Aug 30, 2011

Comment author: ripoche

The OMake issue I was talking about is
http://bugzilla.metaprl.org/cgi-bin/show_bug.cgi?id=708

@vicuna
Copy link
Author

vicuna commented Oct 17, 2011

Comment author: ripoche

Although I kept the OCaml patch, the OMake freezes turned out to be something else.

Sorry for the noise.

@vicuna
Copy link
Author

vicuna commented Mar 15, 2012

Comment author: @xavierleroy

In the absence of new information, I'm setting this PR to "no change required". As far as I can see, the currently-implemented behavior is consistent between Win32 and Unix.

@vicuna vicuna closed this as completed Dec 11, 2015
@vicuna vicuna added the bug label Mar 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant