Improve I/O error handling #3321

danpat · 2016-11-16T06:05:04Z

Issue

This PR is to address #3047. It adds some basic wrappers around our file I/O which include consistent error handling and useful exception messages on error.

No file formats are changed.

Tasklist

add regression / cucumber cases (see docs/testing.md)
review
adjust for comments

TheMarex · 2016-11-19T14:36:02Z

This only needs tests? These should be added here:

https://github.com/Project-OSRM/osrm-backend/blob/master/features/options/contract/files.feature#L18-L22

duizendnegen · 2016-11-26T08:02:11Z

I'll try and open a branch to create PRs against this PR, not sure if I'll manage to find time in the coming days though. (Edit: it looks like all the ifstream usage is already implemented; is the next step then a FileWriter?)
Just to align: what about the usage of libosmium io? Will this also be refactored into using the FileReader classes? e.g. https://github.com/Project-OSRM/osrm-backend/blob/master/src/extractor/extractor.cpp#L131

ghoshkaj · 2016-11-28T19:00:06Z

@danpat for review please!!

ghoshkaj · 2016-11-28T22:05:52Z

Benchmark on my machine, extracting California:

FileReader Branch

real       7m6.943s
user       13m11.849s
sys        1m53.742s

Master

real       7m0.821s
user       12m53.339s
sys        1m49.112s

Benchmark on @danpat's machine:

FileReader Branch

real     6m8.743s
user     10m9.099s
sys      0m18.222s

Master

real     6m18.034s
user     10m16.862s
sys      0m18.979s

ghoshkaj · 2016-11-30T00:15:33Z

@danpat review por favor?

ghoshkaj · 2016-11-30T15:17:47Z

ooor, because Dan and I have been working on this pretty much the whole time together, anybody else want to review? :D @TheMarex mayhaps?

daniel-j-h · 2016-11-30T14:49:40Z

include/extractor/raster_source.hpp


        BOOST_ASSERT(buffer.size() > 1);
-        stream.read(&buffer[0], static_cast<std::streamsize>(buffer.size()));
+
+        file_reader.ReadInto(&buffer[0], buffer.size());


Instead of the pattern

string b; b.resize(reader.Size()) reader.ReadInto(&b[0], b.size());

why not abstract over this to let the reader already return a type filled with the contents?

Ticketing and will tackle in a different PR.

daniel-j-h · 2016-11-30T14:52:37Z

include/storage/dedicated_io.hpp

+{
+namespace storage
+{
+namespace dedicated_io


How is dedicated io different to the storage::io namespace? and how is it related to util::io?

@daniel-j-h dedicated_io could have a better name. It's basically the parts of FileReader that is reading a specialized type of input. For example, readHSGRHeader which "Reads the checksum, number of nodes and number of edges written in the header file of a .hsgr file and returns them in a HSGRHeader struct". This is not your typical filestream that simply opens up a file, verifies a fingerprint or not and returns the FileReader object.

This file has readHSGRHeader, readPropertiesCount, readDatasourceIndexes, readEdges, readNodes, readDatasourceNames and the assoicated structs.

It strikes me that these are all related to reading graphs and graph components. So perhaps I can rename this header to graph_io or something similar?

daniel-j-h · 2016-11-30T14:53:30Z

include/storage/dedicated_io.hpp

+    {
+        util::SimpleLogger().Write(logWARNING) << ".hsgr was prepared with different build.\n"
+                                                  "Reprocess to get rid of this warning.";
+    }


I was under the impression the FileReader is now doing all the checks and we don't have to do this ourselves?

The problem is that the FileReader doesn't do this specific fingerprint test: if (!fingerprint_loaded.TestGraphUtil(fingerprint_valid)) so we have to do it here.

daniel-j-h · 2016-11-30T14:55:07Z

include/storage/dedicated_io.hpp

+    input_file.ReadInto(header.number_of_edges);
+
+    BOOST_ASSERT_MSG(0 != header.number_of_nodes, "number of nodes is zero");
+    // number of edges can be zero, this is the case in a few test fixtures


what about checking: if (num edges > 0) assert num nodes > 0; ?

@daniel-j-h I changed this to what you suggested above, but I'm missing context. Would you please explain? I'd like to know why BOOST_ASSERT_MSG(0 != header.number_of_nodes, "number of nodes is zero"); made sense at all.

daniel-j-h · 2016-11-30T14:56:50Z

include/storage/dedicated_io.hpp

+    BOOST_ASSERT(node_buffer);
+    BOOST_ASSERT(edge_buffer);
+    input_file.ReadInto(node_buffer, number_of_nodes);
+    input_file.ReadInto(edge_buffer, number_of_edges);


I assume the reader handles a count of zero?

Yup! By returning:

osrm-backend/include/storage/io.hpp

Line 66 in 73e365d

if (count == 0)

daniel-j-h · 2016-11-30T15:23:35Z

src/contractor/contractor.cpp

-        if (!turn_penalty_file)
-            throw util::exception{"Unable to open turn penalty file " + filename};
-
+        storage::io::FileReader turn_penalty_file_reader(std::string(filename),


Well this is weird. Earlier, both this and the segment_speed_filenames[idx] were failing to compile because filename was a char* and not a string. And therefore would not compile without being cast as a string. Now it seems to be compiling just fine. So I'll remove it.

daniel-j-h · 2016-11-30T15:23:42Z

src/contractor/contractor.cpp

        TurnPenaltySourceFlatMap local;

        std::uint64_t from_node_id{};
        std::uint64_t via_node_id{};
        std::uint64_t to_node_id{};
        double penalty{};

-        for (std::string line; std::getline(turn_penalty_file, line);)
-        {
+        std::for_each(turn_penalty_file_reader.GetLineIteratorBegin(), turn_penalty_file_reader.GetLineIteratorEnd(), [&](const std::string &line) {


daniel-j-h · 2016-11-30T15:24:35Z

unit_tests/util/io.cpp

@@ -28,4 +30,77 @@ BOOST_AUTO_TEST_CASE(io_data)
    BOOST_CHECK_EQUAL_COLLECTIONS(data_out.begin(), data_out.end(), data_in.begin(), data_in.end());
 }

+BOOST_AUTO_TEST_CASE(io_nonexistent_file)
+{
+    static std::string IO_NONEXISTENT_FILE = "non_existent_test_io.tmp";


daniel-j-h · 2016-11-30T15:25:14Z

unit_tests/util/io.cpp

+    } catch (const osrm::util::exception &e) {
+      std::cout << e.what() << std::endl;
+      BOOST_REQUIRE(std::string(e.what()) == "Error opening non_existent_test_io.tmp:No such file or directory");
+    }


I think there is a macro CHECK_THROWS of something like that for this exact use-case

daniel-j-h · 2016-11-30T15:26:01Z

unit_tests/util/io.cpp

+      data_in.resize(153);
+
+      for (std::size_t i = 0; i < data_in.size(); ++i)
+          data_in[i] = i;


check out std::iota

vector<int> v(n); iota(begin(v), end(v));

cool! done!

danpat added the Work In Progress label Nov 16, 2016

danpat added this to the 5.5.0 milestone Nov 16, 2016

TheMarex changed the title ~~[wip] Improve I/O error handling~~ Improve I/O error handling Nov 17, 2016

ghoshkaj force-pushed the fix/3047 branch from 86e74b2 to e98a136 Compare November 22, 2016 01:22

ghoshkaj mentioned this pull request Nov 23, 2016

(Do not merge) Data serialization #3275

Closed

3 tasks

ghoshkaj force-pushed the fix/3047 branch from e98a136 to 641c13d Compare November 23, 2016 16:24

duizendnegen mentioned this pull request Nov 26, 2016

New OSRM data format #2242

Closed

ghoshkaj force-pushed the fix/3047 branch from c279e35 to 3d14349 Compare November 28, 2016 14:29

ghoshkaj mentioned this pull request Nov 28, 2016

circular header dependencies #3371

Closed

4 tasks

ghoshkaj force-pushed the fix/3047 branch 2 times, most recently from 38d08ce to 64151ee Compare November 30, 2016 00:14

daniel-j-h requested changes Nov 30, 2016

View reviewed changes

duizendnegen mentioned this pull request Nov 30, 2016

FileWriter and some FileReader refactoring #3383

Merged

3 tasks

ghoshkaj force-pushed the fix/3047 branch 2 times, most recently from 4f4c6b3 to 025cbb6 Compare November 30, 2016 18:26

danpat force-pushed the fix/3047 branch 2 times, most recently from 3cf65f8 to b63f8b6 Compare December 1, 2016 01:33

Consolidate file reading through the new FileReader class/interface.

edeb6c6

danpat force-pushed the fix/3047 branch from b63f8b6 to edeb6c6 Compare December 1, 2016 02:38

danpat added Ready To Merge and removed Work In Progress labels Dec 1, 2016

danpat merged commit 5a31101 into master Dec 1, 2016

danpat deleted the fix/3047 branch December 1, 2016 03:08

danpat mentioned this pull request Dec 1, 2016

Failure to open streams does not report a very helpful error #3047

Closed

This was referenced Dec 2, 2016

Better error message when missing .osrm.edge_penalties file #3114

Closed

read() and write() may fail, we do not check for this #1682

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve I/O error handling #3321

Improve I/O error handling #3321

danpat commented Nov 16, 2016 •

edited by ghoshkaj

Loading

TheMarex commented Nov 19, 2016

duizendnegen commented Nov 26, 2016 •

edited

Loading

ghoshkaj commented Nov 28, 2016

ghoshkaj commented Nov 28, 2016

ghoshkaj commented Nov 30, 2016

ghoshkaj commented Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016 •

edited

Loading

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

daniel-j-h Nov 30, 2016

daniel-j-h Nov 30, 2016

ghoshkaj Nov 30, 2016

Improve I/O error handling #3321

Improve I/O error handling #3321

Conversation

danpat commented Nov 16, 2016 • edited by ghoshkaj Loading

Issue

Tasklist

TheMarex commented Nov 19, 2016

duizendnegen commented Nov 26, 2016 • edited Loading

ghoshkaj commented Nov 28, 2016

ghoshkaj commented Nov 28, 2016

ghoshkaj commented Nov 30, 2016

ghoshkaj commented Nov 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghoshkaj Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danpat commented Nov 16, 2016 •

edited by ghoshkaj

Loading

duizendnegen commented Nov 26, 2016 •

edited

Loading

ghoshkaj Nov 30, 2016 •

edited

Loading