Detect IO errors in save #698

Merged
merged 7 commits into from Mar 10, 2014

Conversation

Projects
None yet
3 participants
Owner

rimms commented Feb 27, 2014

If system is disk-full, save return no error (And, broken file is outputted) in 0.5.2.

This patch will solve that problem.

  • Detect i/o error and raise an exception
  • Print log about the cause of exception using errno

And I changed log message of load as same with save.

@kmaehashi kmaehashi and 1 other commented on an outdated diff Feb 27, 2014

jubatus/server/framework/server_base.cpp
std::ifstream ifs(path.c_str(), std::ios::binary);
if (!ifs) {
+ LOG(ERROR) << "failed to load: " << path;
@kmaehashi

kmaehashi Feb 27, 2014

Owner
  • Please use different error message for each different line of LOG (e.g., the same message in 11 lines below).
  • Generally I think it's not good to print logs when throwing exception; exceptions are automatically logged.
@rimms

rimms Feb 28, 2014

Owner

OK, I will fix.

@kmaehashi kmaehashi and 1 other commented on an outdated diff Feb 27, 2014

jubatus/server/framework/server_base.cpp
LOG(ERROR) << "failed to load: " << path;
- LOG(ERROR) << e.what();
@kmaehashi

kmaehashi Feb 27, 2014

Owner

Why not catch std::exception and print the message? I think it will cover most of things.

@rimms

rimms Feb 28, 2014

Owner

All exceptions will be caught in rpc_server.cpp#L48 and that cause will be printed.

I catch all type exceptions only to print "load is failed".

@kmaehashi kmaehashi and 1 other commented on an outdated diff Feb 27, 2014

jubatus/server/framework/server_base.cpp
- // use gcc-specific extension
- int fd = static_cast<__gnu_cxx::stdio_filebuf<char> *>(ofs.rdbuf())->fd();
- if (flock(fd, LOCK_EX | LOCK_NB) < 0) { // try exclusive lock
- throw
- JUBATUS_EXCEPTION(core::common::exception::runtime_error(
- "cannot get the lock of file; any RPC is saving to same file?")
- << core::common::exception::error_file_name(path)
- << core::common::exception::error_errno(errno));
- }
+ std::ofstream ofs;
+ ofs.exceptions(std::ios_base::failbit | std::ios_base::badbit);
@kmaehashi

kmaehashi Feb 27, 2014

Owner

Isn't it nice to use it when loading models too?

@rimms

rimms Feb 28, 2014

Owner

That's right! I will add this idea in this patch.

Owner

suma commented Feb 28, 2014

Is incomplete saved file deleted when error occured?

Owner

rimms commented Feb 28, 2014

@suma I didn't consider about that. I think deleting incomplete saved file is better for users. I will add this idea in this patch. Thank you!

Owner

rimms commented Mar 5, 2014

I updated this patch.

Log messages are followings.

  • Cannot open file (Permission Denied)
I0303 17:23:13.965152 21952 server_base.cpp:89] starting save to /tmp/xx.x.xxx.xxx_9199_jubatus_hoge.jubatus
W0303 17:23:13.965250 21952 rpc_server.cpp:46] Dynamic exception type: jubatus::core::common::exception::runtime_error::what: cannot open output file
    #0 [jubatus::core::common::exception::error_file_name_*] = /tmp/jubatus/xx.x.xxx.xxx_9199_jubatus_hoge.jubatus
    #0 [jubatus::core::common::exception::error_errno_*] = Permission denied (13)
    #0 [jubatus::core::common::exception::error_at_file_*] = ../jubatus/server/framework/server_base.cpp
    #0 [jubatus::core::common::exception::error_at_line_*] = 97
    #0 [jubatus::core::common::exception::error_at_func_*] = virtual bool jubatus::server::framework::server_base::save(const string&)
  • Cannot write file (Disk-full)
I0305 11:55:51.812046  8236 server_base.cpp:90] starting save to /tmp/xx.x.xxx.xxx_9199_jubatus_hoge.jubatus
W0305 11:55:51.812429  8236 rpc_server.cpp:46] Dynamic exception type: jubatus::core::common::exception::runtime_error::what: cannot write output file
    #0 [jubatus::core::common::exception::error_file_name_*] = /tmp/xx.x.xxx.xxx_9199_jubatus_hoge.jubatus
    #0 [jubatus::core::common::exception::error_errno_*] = No space left on device (28)
    #0 [jubatus::core::common::exception::error_at_file_*] = ../jubatus/server/framework/server_base.cpp
    #0 [jubatus::core::common::exception::error_at_line_*] = 123
    #0 [jubatus::core::common::exception::error_at_func_*] = virtual bool jubatus::server::framework::server_base::save(const string&)

This should be "input file" instead of "output file".

Owner

rimms replied Mar 6, 2014

I fixed in 396e189.

Owner

kmaehashi commented Mar 7, 2014

I have no additional comments. Could you please merge if you're OK @suma?

suma was assigned by kmaehashi Mar 7, 2014

kmaehashi added this to the 0.5.3 milestone Mar 7, 2014

Owner

suma commented Mar 7, 2014

Thanks @kmaehashi , I'll check the behaviors and then merge at the beginning of next week.

Owner

suma commented Mar 10, 2014

LGTM, I could verified behaviros (Premission denied and Disk-full).
I'll add #include <stdio.h> for remove(3).

suma merged commit 11f35d6 into jubatus:develop Mar 10, 2014

1 check passed

default The Travis CI build passed
Details

rimms deleted the rimms:fearute/detect_ioerrors branch Apr 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment