Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR #490 but for develop #491

Merged
merged 1 commit into from
Sep 6, 2020
Merged

PR #490 but for develop #491

merged 1 commit into from
Sep 6, 2020

Conversation

drdrew42
Copy link
Member

I have been running into sporadic issues with AskSage problems. They seemed to randomly fail when trying to connect to the Sage Cell Server.

It turns out the response header gives plenty of opportunity to match "100":

checking for header:  HTTP/2 200 
date: Fri, 17 Jul 2020 16:51:30 GMT
content-type: application/json; charset=UTF-8
content-length: 437
set-cookie: __cfduid=dc7f19b456d5fb97ea5fbf50f3f71c4fa1595004689; expires=Sun, 16-Aug-20 16:51:29 GMT; path=/; domain=.sagemath.org; HttpOnly; SameSite=Lax
vary: Accept-Encoding
cf-cache-status: DYNAMIC
cf-request-id: 03ff493591000074018d33d200000001
expect-ct: max-age=604800, report-uri="https://report-uri.cloudflare.com/cdn-cgi/beacon/expect-ct"
server: cloudflare
cf-ray: 5b4577cf49197401-IAD at /Users/drdrew42/mojo/renderv2/lib/RenderApp/Controller/../../PG/lib/WeBWorK/PG/IO.pm line 327

So, I've tightened up the regex matching for the header to more accurately identify HTTP 100 and HTTP 200 responses.

@drgrice1
Copy link
Member

This looks good. It certainly is quite likely that the cf-request-id would mess up the check in the current code. In my testing that is the primary culprit, but it seems that the cookie and cf-ray could do it also. In any case, there is definitely plenty of opportunity for an incorrect match. I saw this occur frequently in my initial testing with the current code, and the updated regular expression check fixes it. It was clearly a problem checking the entire header block for a 100 or 200. The new regexp restricts a match to a single line of the header block.

It seems that the query_sage_server subroutine could use some clean up. There is some cruft in there that makes reading the code a bit confusing. Lines 366, 367, 369, 382, and 383 should be deleted. They seem to be left over from some earlier removed approach, and aren't used or meaningful.

Copy link
Member

@taniwallach taniwallach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and it fixes the reported issues. More details in the discussion at #490 which is a hotfix to master (currently WW 2.15).

@drgrice1 drgrice1 merged commit bcba5ea into openwebwork:develop Sep 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants