Re: [Boost-bugs] [Boost C++ Libraries] #1002: [iostreams] close_impl<closable_tag> does not comply with spec

Subject: Re: [Boost-bugs] [Boost C++ Libraries] #1002: [iostreams] close_impl<closable_tag> does not comply with spec
From: Boost C++ Libraries (noreply_at_[hidden])
Date: 2007-12-03 02:28:01

#1002: [iostreams] close_impl<closable_tag> does not comply with spec
--------------------------------+-------------------------------------------
  Reporter:  chad_at_[hidden]  |       Owner:  turkanis 
      Type:  Bugs               |      Status:  assigned 
 Milestone:                     |   Component:  iostreams
   Version:                     |    Severity:  Problem  
Resolution:                     |    Keywords:           
--------------------------------+-------------------------------------------
Changes (by turkanis):

  * owner:  dgregor => turkanis
  * status:  new => assigned

Comment:

 The following analysis was provide by Chad Walters. I am in the process of
 implementing his proposal #3, below.

 -------------------

 I have come to the conclusion that there may
 need to be a somewhat larger set of changes than the patches that we have
 been considering so far, at least for the long run. Let me walk through
 that
 reasoning and see if you agree.

 Here are all the cases covered by the code in close.hpp, matched up
 against
 the semantics table and the rest of the close function documentation.

 1. Non-closable devices and filters cannot end up in the closable_tag
 specialization of close_impl. They don't get closed by the close function
 --
 they just get flushed.

 2. Closable bidirectional devices and filters cannot end up in the
 closable_tag specialization of close_impl -- they go to the two_sequence
 version instead. They always get closed by the close function, regardless
 of
 the value of the which parameter, and invoke the device or filter's
 close(which) method.

 3. Closable dual_use filters and details::teed_sequence devices and
 filters
 follow the same path in the code as #2 above. These are not covered
 explicitly in the documentation of the close function at all. I need to
 understand the use of dual_use a little more to really understand this
 case.
 I also note that teed_sequence appears only in tee.hpp and is actually
 commented out in both cases. Furthermore, close.hpp is the only place in
 the
 library where teed_sequence is used. I think this notion is basically
 deprecated at this point and should be removed.

 4. A closable device or filter that is input only should be closed
 according
 to whether (which & ios_base::in) != 0, and invoke the close() method.

 5. A closable device or filter that is output only should be closed
 according to whether (which & ios_base::out) != 0, and invoke the close()
 method. This is the case that I was trying to patch up -- the code was
 doing
 the wrong thing here when (which & ios_base::out) != 0 && (which &
 ios_base::in) != 0. The documentation under "When is close invoked?"
 implies
 to some extent that the close function should only ever be invoked with
 the
 which parameter equal to exactly one of ios_base::in or ios_base::out, but
 not both. But this case was happening nonetheless.

 6. A closable device or filter can be input and output but not be
 bidirectional (like the memory mapped file, as you point out). This means
 that it controls a single sequence. The semantics table implies that this
 sequence should get closed when (which & ios_base::out) != 0, just like
 case
 #4 above here. In addition, this would be consistent with the sequencing
 in
 the "When should close be invoked?" discussion for filtering_streambuf and
 filtering_stream. The close function should get invoked twice for the
 single
 sequence device or filter, first with ios_base::in and then with
 ios_base::out. Since this is a single sequence, it probably shouldn't be
 closed until the second invocation (ie: the one with ios_base::out).

 OK, let's pause here before pressing on... Hopefully everything so far
 makes
 good sense.

 Based on the above, I think that the existing code in close.hpp would
 basically have been correct if the which parameter could only be exactly
 one
 of ios_base::in or iosbase::out, but not both. As you pointed out, the
 tee_filter and tee_device code can invoke the close function with the
 which
 parameter equal to (ios_base::in | ios_base::out). This conflict is the
 direct source of the bug I reported.

 I think that the true problem actually lies a little deeper. It seems to
 me
 that the fundamental issue stems from the fact that the Closable concept
 requires that a Filter or Device provide different versions of close(),
 depending on whether it controls a single sequence or two sequences (as
 described under "Examples" in the Closable documentation): if they contain
 a
 single sequence, then they need to have a close method that doesn't have
 an
 ios_base::mode parameter; and if they contain two sequences, they must
 have
 a close method with an ios_base::mode parameter.

 However, there are several devices and filters in the iostreams library
 that, for simplicity's sake, provide a close method with an optional
 default
 value for the which parameter so that they can wrap other devices or
 filters
 without caring about whether the wrapped device or filter has one or two
 sequences. Furthermore, some of them, like tee_filter and tee_device, set
 that which parameter to (ios_base::in | ios_base::out). This means that
 close_impl can get called with this unexpected value.

 There are 3 different possible avenues to fixing the problems with
 closing,
 with increasing levels of difficulty and architectural cleanliness.

 1. Just make the spot fix for the particular bug that I encountered in
 close.hpp; that is, make the close_impl for non-bidirectional closable
 devices and filters behave as if (ios_base::in | ios_base::out) were a
 valid
 input for the which parameter. I would adjust your patch by changing the
 body of your should_close function: just remove the "if (hasIn && hasOut)
 return false;" partl otherwise the wrong thing happens for case #6 above.
 I
 would also leave the documentation of the close function semantics
 untouched
 (ie: rollback the change you made) -- it is correctly stating the cases.
 Also, no need to change the default parameter in tee_filter and tee_device
 (in case something is done WRT issue #791).

 2. Change all the devices and filters in the iostreams library that
 provide
 a default value for the which parameter on their close methods to support
 separate close methods, one without the which parameter and one with the
 which parameter. Make sure the right version gets invoked by all the
 callers
 in the library, depending on whether the underlying stream has one or two
 sequences. There probably need to be specializations of tee_device and
 tee_filter, depending on whether the underlying Devices have one or two
 sequences.

 3. Do all the work for #2 above. In addition, change the Closable concept
 and the close function so that only exactly two values for the which
 parameter are possible. Simply changing to a bool would really be the
 simplest option here, but I suppose other more complex template-based
 things
 could work as well.

 Ordinarily, I'd recommend #1 for a short term fix (no API changes) for the
 next bugfix release and #3 for the next major revision. However, we might
 be
 too close to 1.35.0 to do the changes for #3 in time. I guess #1 for
 1.35.0
 would work, with #3 for 1.36.0 in the (distant?) future would be the
 minimum
 desirable outcome.

-- 
Ticket URL: <http://svn.boost.org/trac/boost/ticket/1002#comment:4>
Boost C++ Libraries <http://www.boost.org/>
Boost provides free peer-reviewed portable C++ source libraries.