search

dmwm / DBSClient

Apache License 2.0
3 stars 8 forks source link

fixed the error handling when html was in the error body. #43

Closed yuyiguo closed 3 years ago

yuyiguo commented 3 years ago

DBS server does not form a error in http. This is from either the underline framework or front end.

yuyiguo commented 3 years ago

@amaltaro @vkuznet PR #26/#27 hit the JSONDecodeError. There are some errors like below that are not in json format. These errors are from either underline framework or FE because DBS server does not format error into html. Please review the PR. I have push 3.17.7 in pipy, but hit the error during testing. Will make a new release as soon as I get this PR in.

<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML 2.0//EN">
<html><head>
<title>502 Proxy Error</title>
</head><body>
<h1>Proxy Error</h1>
<p>The proxy server received an invalid
response from an upstream server.<br />
The proxy server could not handle the request <em><a href="/auth/complete/dbs/int/global/DBSReader/fileparentsbylumi">POST&nbsp;/auth/complete/dbs/int/global/DBSReader/fileparentsbylumi</a></em>.<p>
Reason: <strong>Error reading from remote server</strong></p></p>
</body></html>
vkuznet commented 3 years ago

@yuyiguo , why do you apply these PRs to main branch, they are against DBSClient4go branch which you don't need to use in your testing since you're still making release for python server.

The error indeed comes from FE. The problem is that current DBSClient code does not handle HTTP errors coming from FE server, but it does talk to FE in order to reach DBS backend server. What should be correctly added to DBSClient is a check for HTTP Code and it should ONLY perform JSON decoding if HTTP code is 200. For all other codes it should print appropriate message.

Moreover, the error you show now clearly indicates the reason which is desired to know when some HTTP request fails. In this case the POST request to DBS BE server fails for some reason. Since error says that it can't reach BE server the actual issue that I made in PR #26 the error of assigning self.reader=None which was fixed in PR #29.

To sum up:

vkuznet commented 3 years ago

But the error now may occur now if something will be wrong between FE and BE communication and for that, as I wrote, the DBSClient code should be corrected to check for HTTP code before decoding JSON results. If you want this fix I can provide it.

yuyiguo commented 3 years ago

@vkuznet I need to build a new release to fix the current issue in order for @amaltaro to continue testing DBSClient. In addition, the change is concentrated in one file so future merger will not be a be problem. We have to go parallely in both main and DBSClent4go in order to make things going. If you want, I can make a PR against DBSClient4Go, just like Alan did.

The error from FE (502 Proxy Error) is not something new. This was because the 300 second limit of the FE was passed. The tests was doing a migration of a dataset and that might take way over 300 second. I don't want to change anything on the server side at this time and the clients knew how to handle this error. This PR fixed the problem and let us to continue. I will commit this PR and build a new release unless I find something wrong.

vkuznet commented 3 years ago

Here is a suggestion how to better deal with errors: https://github.com/dmwm/DBSClient/pull/45

vkuznet commented 3 years ago

@yuyiguo I was confused by your statement: PR #26/#27 hit the JSONDecodeError. If you're doing new release using main branch I doubt that you need these PRs at all.

yuyiguo commented 3 years ago

@vkuznet "#26/#27" was a typo, sorry about it. I meant "#36/#37" that was created by Alan when he tried to fix the problem. This PR already handled the issue. I don't think your PR #45 will solve the problem, but thank you for your try. I am going to commit this PR to main and build release, I will make the same PR against DBSClient4Go branch so we will keep the two branch in sync.