search

clj-commons / aleph

Asynchronous streaming communication for Clojure - web server, web client, and raw TCP/UDP
http://aleph.io
MIT License
2.54k stars 241 forks source link

client: ensure response in case of Exception #592

Closed arnaudgeiser closed 2 years ago

arnaudgeiser commented 2 years ago

Context

This PR ensures a response is always given to the client even when an Exception is thrown.

It has been reported by the following issue : #542

KingMob commented 2 years ago

@arnaudgeiser

I've been reading Oleksii's old code today in the error-handling branch-1.0.0 branch, and unfortunately, there's a lot of diffs. On the one hand, he wrote many test cases, made commits already for some of the issues that we're now addressing, and was (honestly) more familiar with the whole Aleph code base than either of us (well, me, for sure). On the other hand, the branch was never merged, so I don't know which ideas he meant to keep working on and which he considered done.

I think to the extent that he wrote tests, and his code passes those tests, we should defer to him in cases where his code diverges from ours, and think very hard about when we shouldn't use his code. I personally do not have the time to master Aleph's code base now and work on my own job. If you do, and can write up why we should or shouldn't use his code in this case, go for it, and let me know.

One possibility I'm mulling is to work through his 1.0.0 branch, commit-by-commit, cherry-picking/merging into the master branch and using each as an opportunity to understand a bit more. Most of the commits seem independent, with a few exceptions.

arnaudgeiser commented 2 years ago

I've been reading Oleksii's old code today in the error-handling branch-1.0.0 branch, and unfortunately, there's a lot of diffs. On the one hand, he wrote many test cases, made commits already for some of the issues that we're now addressing, and was (honestly) more familiar with the whole Aleph code base than either of us (well, me, for sure). On the other hand, the branch was never merged, so I don't know which ideas he meant to keep working on and which he considered done.

Maybe it's not the right place to have this discussion, but my personal feeling around this branch is it will never be merged. As you mentioned, there are too many changes we are not comfortable enough with. Even if we decide to merge it, we will never be sure that the open issues have been resolved, which is even more annoying. On the current state, only Oleksii and Zach can "quickly" determine if merging the branch is safe or not. It's been two years from now, not sure everything is still fresh on their head. On my side, I consider the work done on the 1.0.0 (and the error-handling one) branch as a source of inspiration.

I think to the extent that he wrote tests, and his code passes those tests, we should defer to him in cases where his code diverges from ours, and think very hard about when we shouldn't use his code. I personally do not have the time to master Aleph's code base now and work on my own job. If you do, and can write up why we should or shouldn't use his code in this case, go for it, and let me know.

All for it. However, the commit you pointed has only one test while you would expect two (this line probably has no test: https://github.com/clj-commons/aleph/commit/c9a6739db61948d64ef4c98e3df1d31753987fc3#diff-2d6587452350a3603e05f940d19be2b98e6377dfc7efc3dc1eb099ff332e4b3bR631). Furthermore, I don't know if the test he added cover the exact same issue as mine as it depends on another commit (the fact everything is wrapped into a try).

For the current PR, I can switch d/error-deferred ex instead of ex though.

One possibility I'm mulling is to work through his 1.0.0 branch, commit-by-commit, cherry-picking/merging into the master branch and using each as an opportunity to understand a bit more. Most of the commits seem independent, with a few exceptions.

:+1: It would be great whether we could correlate the commits with the open issues. It's already a lot of work I know, but people using Aleph client knows it contains some really annoying bugs, in particular the following one : https://github.com/clj-commons/aleph/issues/532

Some of us are using Aleph server which is perfectly fine in the current state, but ditch the client one because of the locks in favor of something else (clj-http, hato, telex etc...)

KingMob commented 2 years ago

Yeah, sorry if I wasn't clear, but I agree the 1.0.0 branch will never be merged as is. In general, as PRs get larger, the review time and failure risk go up exponentially.

I get that this is a pain point for you (and others), let me take a fresh look and try to make some headway.

arnaudgeiser commented 2 years ago

If I can be of any help, please do not hesitate.

KingMob commented 2 years ago

Fixes #542