You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@beam.apache.org by Lukasz Cwik <lc...@google.com> on 2018/02/01 02:09:54 UTC
Re: why org.apache.beam.sdk.util.UnownedInputStream fails on close
instead of ignoring it
Yes, people will write bad coders which is why this is there. No, I don't
think swallowing one close is what we want.
In the case where you wants to pass an input/output stream to a library
that incorrectly assumes ownership, the input/output stream should be
wrapped right before the call to the library with a filter input/output
stream that swallows the close and not propagate ignoring this bad behavior
elsewhere.
On Wed, Jan 31, 2018 at 12:04 PM, Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hmm, here we are the ones owning the call since it is in a coder, no? Do
> we assume people will badly implement coders? In this particular case we
> can assume close() will be called by a framework I think.
> What about swallowing one close() and fail on the second?
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau>
>
> 2018-01-31 20:59 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>
>> Because people write code like:
>> myMethod(InputStream in) {
>> InputStream child = new InputStream(in);
>> child.close();
>> }
>>
>> InputStream in = new FileInputStream(... path ...);
>> myMethod(in);
>> myMethod(in);
>>
>> An exception will be thrown when the second myMethod call occurs.
>>
>> Unfortunately not everyone wraps their calls to a coder with an
>> UnownedInputStream or a filter input stream which drops close() calls is
>> why its a problem and in the few places it is done it is used to prevent
>> bugs from creeping in.
>>
>>
>>
>> On Tue, Jan 30, 2018 at 11:29 AM, Romain Manni-Bucau <
>> rmannibucau@gmail.com> wrote:
>>
>>> I get the issue but I don't get the last part. Concretely we can support
>>> any lib by just removing the exception in the close, no? What would be the
>>> issue? No additional wrapper, no lib integration issue.
>>>
>>>
>>> Romain Manni-Bucau
>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>> <http://rmannibucau.wordpress.com> | Github
>>> <https://github.com/rmannibucau> | LinkedIn
>>> <https://www.linkedin.com/in/rmannibucau>
>>>
>>> 2018-01-30 19:29 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>>
>>>> Its common in the code base that input and output streams are passed
>>>> around and the caller is responsible for closing it, not the callee. The
>>>> UnownedInputStream is to guard against libraries that are poorly behaved
>>>> and assume they get ownership of the stream when it is given to them.
>>>>
>>>> In the code:
>>>> myMethod(InputStream in) {
>>>> InputStream child = new InputStream(in);
>>>> child.close();
>>>> }
>>>>
>>>> InputStream in = ...
>>>> myMethod(in);
>>>> myMethod(in);
>>>> When should "in" be closed?
>>>>
>>>> To get around this issue, create a filter input/output stream that
>>>> ignores close calls like on the JAXB coder:
>>>> https://github.com/apache/beam/blob/master/sdks/java/io/xml/
>>>> src/main/java/org/apache/beam/sdk/io/xml/JAXBCoder.java#L181
>>>>
>>>> We can instead swap around this pattern so that the caller guards
>>>> against callees closing by wrapping with a filter input/output stream but
>>>> this costs an additional method call for each input/output stream call.
>>>>
>>>>
>>>> On Tue, Jan 30, 2018 at 10:04 AM, Romain Manni-Bucau <
>>>> rmannibucau@gmail.com> wrote:
>>>>
>>>>> Hi guys,
>>>>>
>>>>> All is in the subject ;)
>>>>>
>>>>> Rational is to support any I/O library and not fail when the close is
>>>>> encapsulated.
>>>>>
>>>>> Any blocker to swallow this close call?
>>>>>
>>>>> Romain Manni-Bucau
>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>>
>>>>
>>>>
>>>
>>
>
Re: why org.apache.beam.sdk.util.UnownedInputStream fails on close
instead of ignoring it
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Yep but makes one other step to work in beam - dont forget you already
passed like 10 steps when you end up on coders.
My view was that the skip first close was a win-win for beam and users bit
technically you are right, users can always do it themselves.
Le 1 févr. 2018 07:16, "Lukasz Cwik" <lc...@google.com> a écrit :
> I'm not sure what you mean by it closes the door since as the caller of
> the library you can create a wrapper filter input stream that ignores close
> calls effectively overriding what happens in the UnownedInputStream.
>
> On Wed, Jan 31, 2018 at 10:08 PM, Romain Manni-Bucau <
> rmannibucau@gmail.com> wrote:
>
>>
>>
>> Le 1 févr. 2018 03:10, "Lukasz Cwik" <lc...@google.com> a écrit :
>>
>> Yes, people will write bad coders which is why this is there. No, I don't
>> think swallowing one close is what we want.
>>
>> In the case where you wants to pass an input/output stream to a library
>> that incorrectly assumes ownership, the input/output stream should be
>> wrapped right before the call to the library with a filter input/output
>> stream that swallows the close and not propagate ignoring this bad behavior
>> elsewhere.
>>
>>
>> Hmm,
>>
>> Elsewhere is nowhere else here since it wouldnt have any side effect
>> except not enforcing another layer and making smoothly work for most
>> mappers.
>>
>> Anyway I can live with it but I'm a bit sad it closes the door to the
>> easyness to write extensions.
>>
>>
>> On Wed, Jan 31, 2018 at 12:04 PM, Romain Manni-Bucau <
>> rmannibucau@gmail.com> wrote:
>>
>>> Hmm, here we are the ones owning the call since it is in a coder, no? Do
>>> we assume people will badly implement coders? In this particular case we
>>> can assume close() will be called by a framework I think.
>>> What about swallowing one close() and fail on the second?
>>>
>>>
>>> Romain Manni-Bucau
>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>> <http://rmannibucau.wordpress.com> | Github
>>> <https://github.com/rmannibucau> | LinkedIn
>>> <https://www.linkedin.com/in/rmannibucau>
>>>
>>> 2018-01-31 20:59 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>>
>>>> Because people write code like:
>>>> myMethod(InputStream in) {
>>>> InputStream child = new InputStream(in);
>>>> child.close();
>>>> }
>>>>
>>>> InputStream in = new FileInputStream(... path ...);
>>>> myMethod(in);
>>>> myMethod(in);
>>>>
>>>> An exception will be thrown when the second myMethod call occurs.
>>>>
>>>> Unfortunately not everyone wraps their calls to a coder with an
>>>> UnownedInputStream or a filter input stream which drops close() calls is
>>>> why its a problem and in the few places it is done it is used to prevent
>>>> bugs from creeping in.
>>>>
>>>>
>>>>
>>>> On Tue, Jan 30, 2018 at 11:29 AM, Romain Manni-Bucau <
>>>> rmannibucau@gmail.com> wrote:
>>>>
>>>>> I get the issue but I don't get the last part. Concretely we can
>>>>> support any lib by just removing the exception in the close, no? What would
>>>>> be the issue? No additional wrapper, no lib integration issue.
>>>>>
>>>>>
>>>>> Romain Manni-Bucau
>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>>
>>>>> 2018-01-30 19:29 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>>>>
>>>>>> Its common in the code base that input and output streams are passed
>>>>>> around and the caller is responsible for closing it, not the callee. The
>>>>>> UnownedInputStream is to guard against libraries that are poorly behaved
>>>>>> and assume they get ownership of the stream when it is given to them.
>>>>>>
>>>>>> In the code:
>>>>>> myMethod(InputStream in) {
>>>>>> InputStream child = new InputStream(in);
>>>>>> child.close();
>>>>>> }
>>>>>>
>>>>>> InputStream in = ...
>>>>>> myMethod(in);
>>>>>> myMethod(in);
>>>>>> When should "in" be closed?
>>>>>>
>>>>>> To get around this issue, create a filter input/output stream that
>>>>>> ignores close calls like on the JAXB coder:
>>>>>> https://github.com/apache/beam/blob/master/sdks/java/io/xml/
>>>>>> src/main/java/org/apache/beam/sdk/io/xml/JAXBCoder.java#L181
>>>>>>
>>>>>> We can instead swap around this pattern so that the caller guards
>>>>>> against callees closing by wrapping with a filter input/output stream but
>>>>>> this costs an additional method call for each input/output stream call.
>>>>>>
>>>>>>
>>>>>> On Tue, Jan 30, 2018 at 10:04 AM, Romain Manni-Bucau <
>>>>>> rmannibucau@gmail.com> wrote:
>>>>>>
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> All is in the subject ;)
>>>>>>>
>>>>>>> Rational is to support any I/O library and not fail when the close
>>>>>>> is encapsulated.
>>>>>>>
>>>>>>> Any blocker to swallow this close call?
>>>>>>>
>>>>>>> Romain Manni-Bucau
>>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>>
>
Re: why org.apache.beam.sdk.util.UnownedInputStream fails on close
instead of ignoring it
Posted by Lukasz Cwik <lc...@google.com>.
I'm not sure what you mean by it closes the door since as the caller of the
library you can create a wrapper filter input stream that ignores close
calls effectively overriding what happens in the UnownedInputStream.
On Wed, Jan 31, 2018 at 10:08 PM, Romain Manni-Bucau <rm...@gmail.com>
wrote:
>
>
> Le 1 févr. 2018 03:10, "Lukasz Cwik" <lc...@google.com> a écrit :
>
> Yes, people will write bad coders which is why this is there. No, I don't
> think swallowing one close is what we want.
>
> In the case where you wants to pass an input/output stream to a library
> that incorrectly assumes ownership, the input/output stream should be
> wrapped right before the call to the library with a filter input/output
> stream that swallows the close and not propagate ignoring this bad behavior
> elsewhere.
>
>
> Hmm,
>
> Elsewhere is nowhere else here since it wouldnt have any side effect
> except not enforcing another layer and making smoothly work for most
> mappers.
>
> Anyway I can live with it but I'm a bit sad it closes the door to the
> easyness to write extensions.
>
>
> On Wed, Jan 31, 2018 at 12:04 PM, Romain Manni-Bucau <
> rmannibucau@gmail.com> wrote:
>
>> Hmm, here we are the ones owning the call since it is in a coder, no? Do
>> we assume people will badly implement coders? In this particular case we
>> can assume close() will be called by a framework I think.
>> What about swallowing one close() and fail on the second?
>>
>>
>> Romain Manni-Bucau
>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>> <https://rmannibucau.metawerx.net/> | Old Blog
>> <http://rmannibucau.wordpress.com> | Github
>> <https://github.com/rmannibucau> | LinkedIn
>> <https://www.linkedin.com/in/rmannibucau>
>>
>> 2018-01-31 20:59 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>
>>> Because people write code like:
>>> myMethod(InputStream in) {
>>> InputStream child = new InputStream(in);
>>> child.close();
>>> }
>>>
>>> InputStream in = new FileInputStream(... path ...);
>>> myMethod(in);
>>> myMethod(in);
>>>
>>> An exception will be thrown when the second myMethod call occurs.
>>>
>>> Unfortunately not everyone wraps their calls to a coder with an
>>> UnownedInputStream or a filter input stream which drops close() calls is
>>> why its a problem and in the few places it is done it is used to prevent
>>> bugs from creeping in.
>>>
>>>
>>>
>>> On Tue, Jan 30, 2018 at 11:29 AM, Romain Manni-Bucau <
>>> rmannibucau@gmail.com> wrote:
>>>
>>>> I get the issue but I don't get the last part. Concretely we can
>>>> support any lib by just removing the exception in the close, no? What would
>>>> be the issue? No additional wrapper, no lib integration issue.
>>>>
>>>>
>>>> Romain Manni-Bucau
>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>> <http://rmannibucau.wordpress.com> | Github
>>>> <https://github.com/rmannibucau> | LinkedIn
>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>
>>>> 2018-01-30 19:29 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>>>
>>>>> Its common in the code base that input and output streams are passed
>>>>> around and the caller is responsible for closing it, not the callee. The
>>>>> UnownedInputStream is to guard against libraries that are poorly behaved
>>>>> and assume they get ownership of the stream when it is given to them.
>>>>>
>>>>> In the code:
>>>>> myMethod(InputStream in) {
>>>>> InputStream child = new InputStream(in);
>>>>> child.close();
>>>>> }
>>>>>
>>>>> InputStream in = ...
>>>>> myMethod(in);
>>>>> myMethod(in);
>>>>> When should "in" be closed?
>>>>>
>>>>> To get around this issue, create a filter input/output stream that
>>>>> ignores close calls like on the JAXB coder:
>>>>> https://github.com/apache/beam/blob/master/sdks/java/io/xml/
>>>>> src/main/java/org/apache/beam/sdk/io/xml/JAXBCoder.java#L181
>>>>>
>>>>> We can instead swap around this pattern so that the caller guards
>>>>> against callees closing by wrapping with a filter input/output stream but
>>>>> this costs an additional method call for each input/output stream call.
>>>>>
>>>>>
>>>>> On Tue, Jan 30, 2018 at 10:04 AM, Romain Manni-Bucau <
>>>>> rmannibucau@gmail.com> wrote:
>>>>>
>>>>>> Hi guys,
>>>>>>
>>>>>> All is in the subject ;)
>>>>>>
>>>>>> Rational is to support any I/O library and not fail when the close is
>>>>>> encapsulated.
>>>>>>
>>>>>> Any blocker to swallow this close call?
>>>>>>
>>>>>> Romain Manni-Bucau
>>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>
>
Re: why org.apache.beam.sdk.util.UnownedInputStream fails on close
instead of ignoring it
Posted by Romain Manni-Bucau <rm...@gmail.com>.
Le 1 févr. 2018 03:10, "Lukasz Cwik" <lc...@google.com> a écrit :
Yes, people will write bad coders which is why this is there. No, I don't
think swallowing one close is what we want.
In the case where you wants to pass an input/output stream to a library
that incorrectly assumes ownership, the input/output stream should be
wrapped right before the call to the library with a filter input/output
stream that swallows the close and not propagate ignoring this bad behavior
elsewhere.
Hmm,
Elsewhere is nowhere else here since it wouldnt have any side effect except
not enforcing another layer and making smoothly work for most mappers.
Anyway I can live with it but I'm a bit sad it closes the door to the
easyness to write extensions.
On Wed, Jan 31, 2018 at 12:04 PM, Romain Manni-Bucau <rm...@gmail.com>
wrote:
> Hmm, here we are the ones owning the call since it is in a coder, no? Do
> we assume people will badly implement coders? In this particular case we
> can assume close() will be called by a framework I think.
> What about swallowing one close() and fail on the second?
>
>
> Romain Manni-Bucau
> @rmannibucau <https://twitter.com/rmannibucau> | Blog
> <https://rmannibucau.metawerx.net/> | Old Blog
> <http://rmannibucau.wordpress.com> | Github
> <https://github.com/rmannibucau> | LinkedIn
> <https://www.linkedin.com/in/rmannibucau>
>
> 2018-01-31 20:59 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>
>> Because people write code like:
>> myMethod(InputStream in) {
>> InputStream child = new InputStream(in);
>> child.close();
>> }
>>
>> InputStream in = new FileInputStream(... path ...);
>> myMethod(in);
>> myMethod(in);
>>
>> An exception will be thrown when the second myMethod call occurs.
>>
>> Unfortunately not everyone wraps their calls to a coder with an
>> UnownedInputStream or a filter input stream which drops close() calls is
>> why its a problem and in the few places it is done it is used to prevent
>> bugs from creeping in.
>>
>>
>>
>> On Tue, Jan 30, 2018 at 11:29 AM, Romain Manni-Bucau <
>> rmannibucau@gmail.com> wrote:
>>
>>> I get the issue but I don't get the last part. Concretely we can support
>>> any lib by just removing the exception in the close, no? What would be the
>>> issue? No additional wrapper, no lib integration issue.
>>>
>>>
>>> Romain Manni-Bucau
>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>> <http://rmannibucau.wordpress.com> | Github
>>> <https://github.com/rmannibucau> | LinkedIn
>>> <https://www.linkedin.com/in/rmannibucau>
>>>
>>> 2018-01-30 19:29 GMT+01:00 Lukasz Cwik <lc...@google.com>:
>>>
>>>> Its common in the code base that input and output streams are passed
>>>> around and the caller is responsible for closing it, not the callee. The
>>>> UnownedInputStream is to guard against libraries that are poorly behaved
>>>> and assume they get ownership of the stream when it is given to them.
>>>>
>>>> In the code:
>>>> myMethod(InputStream in) {
>>>> InputStream child = new InputStream(in);
>>>> child.close();
>>>> }
>>>>
>>>> InputStream in = ...
>>>> myMethod(in);
>>>> myMethod(in);
>>>> When should "in" be closed?
>>>>
>>>> To get around this issue, create a filter input/output stream that
>>>> ignores close calls like on the JAXB coder:
>>>> https://github.com/apache/beam/blob/master/sdks/java/io/xml/
>>>> src/main/java/org/apache/beam/sdk/io/xml/JAXBCoder.java#L181
>>>>
>>>> We can instead swap around this pattern so that the caller guards
>>>> against callees closing by wrapping with a filter input/output stream but
>>>> this costs an additional method call for each input/output stream call.
>>>>
>>>>
>>>> On Tue, Jan 30, 2018 at 10:04 AM, Romain Manni-Bucau <
>>>> rmannibucau@gmail.com> wrote:
>>>>
>>>>> Hi guys,
>>>>>
>>>>> All is in the subject ;)
>>>>>
>>>>> Rational is to support any I/O library and not fail when the close is
>>>>> encapsulated.
>>>>>
>>>>> Any blocker to swallow this close call?
>>>>>
>>>>> Romain Manni-Bucau
>>>>> @rmannibucau <https://twitter.com/rmannibucau> | Blog
>>>>> <https://rmannibucau.metawerx.net/> | Old Blog
>>>>> <http://rmannibucau.wordpress.com> | Github
>>>>> <https://github.com/rmannibucau> | LinkedIn
>>>>> <https://www.linkedin.com/in/rmannibucau>
>>>>>
>>>>
>>>>
>>>
>>
>