-
Notifications
You must be signed in to change notification settings - Fork 176
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
EBADF from Lwt_process.with_* #956
Comments
i don't think it was the root cause because (according to that PR) it broke in 9de01c9 which was in 5.6.0, but we're using 5.5.0. i'm going to use a workaround in Flow to see if I can figure out what the underlying issue that causes the invalid FD is, so that we can hopefully find a repro/write a test. |
Ok, thanks for the info. I'll try to have a look soon to check whether we could forward the original exception and what other issue might be the root cause. |
Summary: we're seeing exceptions like this: ``` Failed to initialize watchman: Unix.Unix_error(Unix.EBADF, "close", "") Raised by primitive operation at Lwt_unix.self_result in file "src/unix/lwt_unix.cppo.ml", line 237, characters 14-31 Re-raised at Watchman.get_sockname.(fun) in file "flow/src/hack_forked/watchman/watchman.ml", line 381, characters 2-934 Re-raised at FileWatcher.WatchmanFileWatcher.watchman#wait_for_init.go_exn.(fun) in file "flow/src/monitor/fileWatcher.ml", line 534, characters 10-1023 ``` `Watchman.get_sockname` calls `LwtSysUtils.exec` which calls `Lwt_process.with_process_full`. `with_process_full` does an `Lwt.finally` to make sure it calls `process#close`. `close` is throwing because the process's fd is invalid (perhaps already closed). The stack traces aren't much to go on, but I suspect this is happening on exceptions. The file descriptor shouldn't be invalid otherwise. That means there's an underlying exception that's getting hidden by the `finally` also throwing. So, here we ignore `EBADF` exceptions from the `close` and rethrow the original exception. I also filed ocsigen/lwt#956 to discuss doing this upstream. Changelog: [internal] Reviewed By: samwgoldman Differential Revision: D37420401 fbshipit-source-id: 0e7ddf41a3f4d13290e49c692be66723eda1f227
Summary: we're seeing exceptions like this: ``` Failed to initialize watchman: Unix.Unix_error(Unix.EBADF, "close", "") Raised by primitive operation at Lwt_unix.self_result in file "src/unix/lwt_unix.cppo.ml", line 237, characters 14-31 Re-raised at Watchman.get_sockname.(fun) in file "flow/src/hack_forked/watchman/watchman.ml", line 381, characters 2-934 Re-raised at FileWatcher.WatchmanFileWatcher.watchman#wait_for_init.go_exn.(fun) in file "flow/src/monitor/fileWatcher.ml", line 534, characters 10-1023 ``` `Watchman.get_sockname` calls `LwtSysUtils.exec` which calls `Lwt_process.with_process_full`. `with_process_full` does an `Lwt.finally` to make sure it calls `process#close`. `close` is throwing because the process's fd is invalid (perhaps already closed). The stack traces aren't much to go on, but I suspect this is happening on exceptions. The file descriptor shouldn't be invalid otherwise. That means there's an underlying exception that's getting hidden by the `finally` also throwing. So, here we ignore `EBADF` exceptions from the `close` and rethrow the original exception. I also filed ocsigen/lwt#956 to discuss doing this upstream. Changelog: [internal] Reviewed By: samwgoldman Differential Revision: D37420401 fbshipit-source-id: de63ab6ed2dd4a43ffe190d31da39bd98633a4fb
circling back with results from our workaround. these are the two underlying exceptions we've gotten reports of so far (still no actual repro unfortunately):
both are coming from this code:
this is the |
I've received reports of
Unix.Unix_error(Unix.EBADF, "close", "")
exceptions that stem from thisprocess#close
:lwt/src/unix/lwt_process.cppo.ml
Line 350 in 7355895
Unfortunately I don't have a repro yet to understand what the process we're calling is doing, but we're calling
Lwt_process.with_process_full
here:https://github.com/facebook/flow/blob/38b41863de17211ae70c8bbc57854fbe7fed1ce3/src/common/lwt/lwtSysUtils.ml#L54
According to close(2),
EBADF
in this case means that the file descriptor that Lwt created internally is invalid. Would it make sense to catch and suppress this exception?Whatever the underlying exception is is being swallowed. Even if close's exception is ignored, it'd be nice to rethrow the original exception with its original backtrace if possible.
The text was updated successfully, but these errors were encountered: