Skip to content

Best practices refactor: Minimize "naked" FDs (Native_handle); prefer Own_native_handle. #157

Description

@ygoldfeld

Right off the bat: This does not reference any known bug, nor would it change any API contracts; the generated code would be close to identical as well.

util::Native_handle is internally used as, while we remain POSIX-only at least, almost as nice equivalent to int (used as FD a/k/a Native_handle). That it is its own type, with certain niceties, is good. However, it does not have auto-closing (::close() in dtor and on overwrite via assignment); nor should it. It is what it is, and that thing is a real thing.

util::Own_native_handle is coming soon (in the mega-PR for #143, main [not only] PR for release 3); it is essentially unique_resource<Native_handle>; it ::close()s FD in dtor and on being overwritten (a-la unique_ptr with ::close()=deleter but sans heap use).

In the massive code review preceding release 3, we found quite a few instances of FD-leaking behaviors (~all along error paths but nevertheless). These bugs have been fixed. In many cases (not all) I did this by internally using Own_native_handle.

However: These bugs would not have existed in the first place, if all internal code followed this rule:

  • Always use Own_native_handle...

  • ...except when some API (that cannot be changed, such as a user-facing API, or a native call) requires a naked FD; then use Native_handle or (eventually) Native_handle::handle_t (i.e., int in POSIX).

  • There is essentially no perf cost; a few = -1 ops would be added, at worst.

  • The code would be cleaner in those areas; in many cases, though it would require some tactical effort, it would also be shorter. (No more "if exiting here, don't forget to un-leak some FD" type stuff.)

Priority: Eh, you know. I'd say do it at first opportunity when dealing with FDs. No need to rock the boat until then; it's not some dire problem (anymore -- those bugs are fixed -- but we don't want to add such problems again from here on either).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    style-refactorAn enhancement mainly for style and/or best practices; maybe minor perf benefits

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions