Merge pull request #3 from awslabs/SetFailureBitIfFileOpenFails

Make sure that failbit is set after unsuccessful is3stream opens

Author: sdavtaker

include/awslabs/enhanced/is3stream.h

@@ -5,6 +5,7 @@
 
 #include <awslabs/enhanced/s3buf.h>
 #include <string>
+#include <string_view>
 #include <istream>
 
 namespace AwsLabs::Enhanced {
@@ -13,6 +14,16 @@ class is3stream : public std::istream {
   std::string _region;
   std::string _bucket_name;
   std::string _object_name;
+protected:
+  /**
+   * Opens the current region/bucket/object and set the iostate
+  */
+  void open(std::ios_base::openmode mode = std::ios_base::in) {
+    if(_s3b->open(_region, _bucket_name, _object_name, mode))
+      std::ios::setstate(goodbit);
+    else
+      std::ios::setstate(failbit);
+  }
 public:
   /**
    * Default constructor for a is3stream not associated to an S3 object
@@ -41,7 +52,8 @@ class is3stream : public std::istream {
  * @return
  */
   is3stream &operator=(is3stream &&is3s) {
-    _s3b->close();
+    if(is_open())
+      _s3b->close();
     _region = is3s._region;
     _bucket_name = is3s._bucket_name;
     _object_name = is3s._object_name;
@@ -63,7 +75,7 @@ class is3stream : public std::istream {
     _region = region;
     _bucket_name = bucket_name;
     _object_name = object_name;
-    _s3b->open(_region, _bucket_name, _object_name, std::ios_base::in);
+    open();
   }
 
   /**
@@ -87,7 +99,7 @@ class is3stream : public std::istream {
    */
   void open(const std::string &object_name) {
     _object_name = object_name;
-    _s3b->open(_region, _bucket_name, _object_name, std::ios_base::in);
+    open();
   }
   /**
    * Opens object_name in previously set region and bucket to read content from it.
@@ -99,7 +111,7 @@ class is3stream : public std::istream {
     _region = region;
     _bucket_name = bucket_name;
     _object_name = object_name;
-    _s3b->open(_region, _bucket_name, _object_name, std::ios_base::in);
+    open();
   }
   /**
    * Returns whether the is3stream is currently associated to an S3 object.
@@ -137,7 +149,12 @@ class is3stream : public std::istream {
   }
 
   virtual ~is3stream() {
-    delete _s3b;
+    try {
+      delete _s3b;
+    } catch(...) {
+      // Don't throw from destructors
+      // https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Re-never-fail
+    }
   }
 };
 

test/test_is3stream.cpp

@@ -7,7 +7,6 @@
 
 #include "gtest/gtest.h"
 #include "test_helpers.h"
-
 namespace {
 class is3sIntegrationTest : public ::testing::Test {
 protected:
@@ -85,4 +84,19 @@ TEST_F(is3sIntegrationTest, CanReadFirstWordFromIs3s) {
   is3s >> result;
   ASSERT_FALSE(result.empty()) << "Second word is also extracted";
 }
+
+TEST_F(is3sIntegrationTest, DetectS3FailureInIs3sConstructor) {
+  // "foo" exists but we do not have permission to read
+  AwsLabs::Enhanced::is3stream is3s(infra.m_region, "foo", infra.m_object_name);
+  // Vary tests between checking for failure and exceptions for greater robustness
+  ASSERT_THROW(is3s.exceptions(std::ios_base::failbit | std::ios_base::badbit), std::ios_base::failure);
+}
+
+TEST_F(is3sIntegrationTest, DetectS3FailureInIs3sOpen) {
+  AwsLabs::Enhanced::is3stream is3s;
+  // "foo" exists but we do not have permission to read
+  is3s.open(infra.m_region, "foo", infra.m_object_name);
+  ASSERT_TRUE(is3s.fail() && !is3s.bad());
+}
+
 }