Modify

Ticket #3517 (closed Bugs: fixed)

Opened 5 years ago

Last modified 4 years ago

[iostreams] stream<file_descriptor_source> closes supplied descriptor

Reported by: Alexander Churanov <alexanderchuranov+boost@…> Owned by: danieljames
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

Change History

comment:1 Changed 4 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 follow-up: ↓ 3 Changed 4 years ago by 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? 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 4 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 4 years ago by danieljames

  • Owner changed from turkanis to danieljames
  • Status changed from new to assigned
  • Milestone changed from Boost 1.41.0 to Boost 1.44.0

comment:5 Changed 4 years ago by danieljames

(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 4 years ago by danieljames

  • Status changed from assigned to closed
  • Resolution set to fixed

(In [63502]) Merge iostreams.

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

comment:7 Changed 4 years ago by danieljames

(In [63679]) Partially merge iostreams.

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

Add a comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
The resolution will be deleted. Next status will be 'reopened'
Author


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

 
Note: See TracTickets for help on using tickets.