Modify

Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3517 closed Bugs (fixed)

[iostreams] stream<file_descriptor_source> closes supplied descriptor

Reported by: Alexander Churanov <alexanderchuranov+boost@…> Owned by: Daniel James
Milestone: Boost 1.44.0 Component: iostreams
Version: Boost 1.39.0 Severity: Problem
Keywords: Cc:

Description

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.

Attachments (0)

Change History (7)

comment:1 Changed 7 years ago by 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.

comment:2 Changed 7 years ago by 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.

comment:3 in reply to:  2 Changed 7 years ago by 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.

comment:4 Changed 7 years ago by Daniel James

Milestone: Boost 1.41.0Boost 1.44.0
Owner: changed from Jonathan Turkanis to Daniel James
Status: newassigned

comment:5 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by Daniel James

Resolution: fixed
Status: assignedclosed

(In [63502]) Merge iostreams.

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

comment:7 Changed 7 years ago by Daniel James

(In [63679]) Partially merge iostreams.

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Daniel James.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.