Skip to content

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

Merged
yuyiguo merged 1 commit intodmwm:mainfrom
yuyiguo:test-3_17_7
Nov 5, 2021
Merged

fixed the error handling when html was in the error body.#43
yuyiguo merged 1 commit intodmwm:mainfrom
yuyiguo:test-3_17_7

Conversation

@yuyiguo
Copy link
Member

@yuyiguo yuyiguo commented Nov 4, 2021

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

@yuyiguo
fixed the error handling when html was in the error body. DBS server …
4011f0a 
…does not form a error in http. This is from either the underline framework or fronte end.
@yuyiguo
Copy link
Member Author

yuyiguo commented Nov 4, 2021

@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
Copy link
Contributor

vkuznet commented Nov 5, 2021

@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
Copy link
Contributor

vkuznet commented Nov 5, 2021

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
Copy link
Member Author

yuyiguo commented Nov 5, 2021

@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
Copy link
Contributor

vkuznet commented Nov 5, 2021

Here is a suggestion how to better deal with errors: #45

@vkuznet
Copy link
Contributor

vkuznet commented Nov 5, 2021

@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
Copy link
Member Author

yuyiguo commented Nov 5, 2021

@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.

@yuyiguo yuyiguo merged commit af432c3 into dmwm:main Nov 5, 2021
@yuyiguo yuyiguo mentioned this pull request Nov 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@yuyiguo @vkuznet

Comments