search

odnoklassniki / one-nio

Unconventional I/O library for Java
Apache License 2.0
655 stars 97 forks source link

Retry deserialize after SerializerNotFoundException #12

Closed QIvan closed 7 years ago

QIvan commented 7 years ago

Issue #9 What about the solution like this? I'll write bechmark in the near future.

P.S. why here getClass() but not cls()

apangin commented 7 years ago

why here getClass() but not cls()

UID should depend not only on the target class, but also on the serializer type itself. There may be multiple serializers for the same class.

apangin commented 7 years ago

This PR does not fundamentally differ from #11. It even has the same problem with recursion. But this does not really matter. The actual reason why I'm not very happy with such solution is its contradiction with the original design.

It's not the job of a stream to handle exceptions. Streams are simple and straightforward: they just decode the given format, and if they can't - they throw an exception. It's up to an application to decide what to do with the exception. Maybe, an application developer prefers fail-fast logic - we don't know.

QIvan commented 7 years ago

Please see last commit.

It's not the job of a stream to handle exceptions.

Now DeserializeStream does not catch exception just tries to enrich exception with an information about what serializer was not found because this is the single place where it could be done. And now I can't find contradiction with

Streams are simple and straightforward: they just decode the given format, and if they can't - they throw an exception.

=)

apangin commented 7 years ago

I'd suggest not to spend time trying to improve the approach which is inherently wrong. Please, no business logic inside serialization streams. Stream is just a data source. Why does it intercept its own errors? Why does it break encapsulation and manipulate the internals of serializers when it should ideally know nothing about them? This 'fix' is made of anti-patterns.

Serializer.deserialize is not an appropriate place for catching errors either. It's just a small utility method, a shortcut for DeserializeStream - its behavior should not differ. Furthermore, the infinite recursion is still possible.

Handling only some exceptions (corner cases) makes API inconsistent. It is highly confusing when a general-purpose API automatically adapts to some classes, but fails to decode other classes.

apangin commented 7 years ago

After all, the purpose of this change remains unclear. I don't see what actual problem it tries to solve.