From: Alexey Spiridonov <lesha@fb.com>
Date: Fri, 8 May 2015 01:28:06 +0000 (-0700)
Subject: Destructor DCHECKs for EBADF
X-Git-Tag: v0.38.0~5
X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=ec3c8f790fc7e8d870d518211e124830c4bdf1d3;p=folly.git

Destructor DCHECKs for EBADF

Summary: This almost always indicates a double-close bug, or a similarly nasty logic error. I don't dare make it a check, we may have some code running in production which would be broken by the CHECK, but this should give us early warning of any such bugs.

Test Plan:
```
fbconfig folly/test:file_test folly/test:file_util_test
fbmake runtests && fbmake runtests_opt
```

This test used to pass until @tudorb made me remove it due to worries about death tests messing with (currently absent) multi-threading:

```
+
+void testDoubleClose() {
+  File f("/dev/null");
+  checkUnixError(close(f.fd()));  // This feels so... wrong!
+  // The destructor will now try to double-close.
+}
+
+TEST(File, DCHECKDoubleClose) {
+#ifndef NDEBUG
+  // This test makes no sense otherwise, since this is a DCHECK.
+  EXPECT_DEATH(testDoubleClose(), "double-close-FD");
+#else
+  // That sound you hear is millions of lemmings falling to their doom.
+  testDoubleClose();
+#endif
+}
```

Reviewed By: tudorb@fb.com

Subscribers: folly-diffs@, yfeldblum, chalfant

FB internal diff: D2055610

Signature: t1:2055610:1431048270:a469d5c1f8182ffb74700908faa022e9613ed383
---

diff --git a/folly/File.cpp b/folly/File.cpp
index 0dece45d..379efe62 100644
--- a/folly/File.cpp
+++ b/folly/File.cpp
@@ -65,7 +65,11 @@ File& File::operator=(File&& other) {
 }
 
 File::~File() {
-  closeNoThrow();  // ignore error
+  auto fd = fd_;
+  if (!closeNoThrow()) {  // ignore most errors
+    DCHECK_NE(errno, EBADF) << "closing fd " << fd << ", it may already "
+      << "have been closed. Another time, this might close the wrong FD.";
+  }
 }
 
 /* static */ File File::temporary() {