[iostreams] stream<file_descriptor_source> closes supplied descriptor

[Alexander Churanov <alexanderchuranov+boost@…>]

The instance of stream <file_descriptor_source> closes supplied file descriptor on destruction even if the second specified argument is 'false'.

Expected behavior: descriptor remains opened after stream destruction

The repro application source is at ​http://alexanderchuranov.com/boost-iostreams-autoclose-bug.cc .

Workaround: call "set_auto_close(false)" right after stream construction.

It is believed that the root cause of the issue is indirect_stream having default constructor always initializing it's "flags_" member to f_auto_close.

[Steven Watanabe]

I think the correct fix is to make close() for file_descriptor a no-op, when you pass in an external file descriptor. This makes file_descriptor's semantics simpler, as it either owns the underlying file descriptor or it doesn't.

[Daniel James]

I think you're right, but I feel cautious about silently changing existing behaviour. I also dislike the boolean parameter. So I'd create a new constructor which takes flags to indicated if the handle should be closed. And maybe close_on_exit if someone wants it. I'd deprecate the existing constructor, so that it's only present if a macro is defined. That way we'd break the compile rather than subtly changing the code. What do you think of something like this? Am I being over-cautious?

namespace boost { namespace iostreams {

enum file_handle_flags {
    close_handle, // maybe owns_handle?
    never_close_handle,
    close_handle_on_exit // Probably shouldn't be public.
};

class file_descriptor {
public:
    typedef char                      char_type;
    typedef [implementation-defined]  category;
    file_descriptor( const std::string& pathname, 
                     std::ios_base::open_mode mode = 
                         std::ios_base::in | std::ios_base::out );

    file_descriptor( int fd, file_handle_flags);

    // Windows-only
    file_descriptor( HANDLE hFile, file_handle_flags);

// Maybe something with a version number...
#if defined(BOOST_IOSTREAMS_USE_DEPRECATED)
    file_descriptor( int fd, bool close_on_exit = false );   

    // Windows-only
    file_descriptor( HANDLE hFile, bool close_on_exit = false );
#endif

    bool is_open() const;
};

} } // End namespace boost::io

It'll be more verbose but I think it's worth it to clear up the ambiguity. Oh, and those constructors should probably have been explicit.

[Steven Watanabe]

Replying to danieljames:

I think you're right, but I feel cautious about silently changing existing behaviour. I also dislike the boolean parameter. So I'd create a new constructor which takes flags to indicated if the handle should be closed. And maybe close_on_exit if someone wants it. I'd deprecate the existing constructor, so that it's only present if a macro is defined. That way we'd break the compile rather than subtly changing the code. What do you think of something like this?

Sounds good to me.

Am I being over-cautious?

I don't think so.

<snip code>

It'll be more verbose but I think it's worth it to clear up the ambiguity. Oh, and those constructors should probably have been explicit.

Yep.

[Daniel James]

(In [63430]) Try to improve file_descriptor's ownership policies. Refs #3517

• Deprecate the old 'close_on_exit' constructors.
• Introduce new constructors from file handle, taking either never_close_handle or close_handle.
• Close current file handle when opening a new file handle, if it would have been closed in the destructor.

[Daniel James]

(In [63502]) Merge iostreams.

• New constructors/open for file descriptors. Fixes #3517.
• Use unique_path instead of tmpnam. Refs #2325.

[Daniel James]

(In [63679]) Partially merge iostreams.

• New constructors/open for file descriptors. Fixes #3517.
• Leaving out changes which depend on filesystem v3.