[jira] [Commented] (SOLR-10779) JavaBinCodec's close/finish pattern is trappy

Previous Topic Next Topic
classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view

[jira] [Commented] (SOLR-10779) JavaBinCodec's close/finish pattern is trappy

JIRA jira@apache.org

    [ https://issues.apache.org/jira/browse/SOLR-10779?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16053397#comment-16053397 ]

Erick Erickson commented on SOLR-10779:

bq: "trappy" is often not a great descriptor

Well, _I_ knew what I meant ;)...

So the pattern of having "marshal" call "finsh" which closes the object is just waiting for abuse when the class implements Closeable. We should use close then.

There are lots of usages like:
JavBinCodec jbc = new JavaBinCodec(...)

relying on the marshal code to close the (through the "finish" method). There's no corresponding call in unmarshal however, leading that pattern to be repeated incorrectly as:

JavBinCodec jbc = new JavaBinCodec(...)

which leaves the object open.

[~varunthacker] Heck, I didn't write it, just discovered it when looking at precommit lint warning ;) But the goal here is to get rid of precommit warnings, changing the class to make those two methods static seems too intrusive at this point. Both methods "do stuff" with the FastOutputStream member variable which is used in various other methods. Whether a thorough review of this entire class would lead to a better design I'll leave for a later battle.

I'll leave the alreadyMarshalled/unMarshalled variables in as they are just asserts for debugging.

I'll remove the close variable.

I see no great reason to backport any of this precommit cleanup to the 6x code line.

> JavaBinCodec's close/finish pattern is trappy
> ---------------------------------------------
>                 Key: SOLR-10779
>                 URL: https://issues.apache.org/jira/browse/SOLR-10779
>             Project: Solr
>          Issue Type: Improvement
>      Security Level: Public(Default Security Level. Issues are Public)
>            Reporter: Erick Erickson
>            Assignee: Erick Erickson
>         Attachments: SOLR-10779.patch, SOLR-10779.patch
> Having the marshal() code call finish which in turn calls close() is trappy. The marshal code is not robust anyway since if there's an exception before the try loop, it will not close the resource.
> Sub task of SOLR-10778

This message was sent by Atlassian JIRA

To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]