[iostreams] close_impl<closable_tag> does not comply with spec

[chad@…]

I experienced a problem where a composite Device inside a tee_device was not getting closed when appropriate.

The root cause of the problem is that the implementation of close.hpp::close_impl<closable_tag>::close() is not in accordance with the documentation (http://www.boost.org/libs/iostreams/doc/functions/close.html).

According to the documentation, my closable composite should be closed when close() is invoked with an open mode of (in|out), but the code as written does not allow for this. The patch is attached.

Thanks to gchen (chengang31@…), who pointed me to the problem location.

[chad@…]

Patch for close_impl<closable_tag>

[Frank Birbacher]

Hopefully fixed by a similar patch, see [40700]. I also updated the documentation to match the implementation.

[Jonathan Turkanis]

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.

[Jonathan Turkanis]

Fixed in [42047].