Fixes so far #271
Merged
Fixes so far #271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Do not attempt to parse the request body on a non-2xx response, which will almost certainly not contain what we expect. Perform the check outside of the request function, to still allow handling non-2xx responses in the rare case when it is necessary.
We can't return a HTTPClientResponse if we are going to read the data from disk.
|
Thanks for your pull request, @CyberShadow! |
Geod24
reviewed
May 27, 2021
source/dlangbot/github_api.d
Outdated
Comment on lines
63
to
64
| else | ||
| if (res.statusCode == 200 /*OK*/) |
There was a problem hiding this comment.
Sorry, what specifically? The else\nif?
There was a problem hiding this comment.
I see this is covered by https://dlang.org/dstyle.html#phobos_whitespace, so I'll fix it.
source/dlangbot/github_api.d
Outdated
Comment on lines
76
to
79
| if (res.statusCode / 100 == 3 && "Location" in res.headers) | ||
| { | ||
| auto location = res.headers["Location"]; | ||
| logInfo(" > Redirect: %s", location); | ||
| return ghGetRequest(applyRelativeURL(url, location)); | ||
| } |
There was a problem hiding this comment.
This is not related to caching is it ?
There was a problem hiding this comment.
Ah, no. I've reused code from dtest.dlang.io. The GitHub API does return redirects in some situations (e.g. https://api.github.com/repos/D-Programming-Language/dmd ) , so I think we do need to handle them in case something gets renamed or moved around in the future.
There was a problem hiding this comment.
Could you split it in its own commit then ?
Geod24
reviewed
May 27, 2021
Cache hits do not count against the API rate-limit, so it is always beneficial to do so. The caching code is reused from https://github.com/CyberShadow/DAutoTest/blob/master/github.d and https://github.com/CyberShadow/ae/blob/next/net/github/rest.d, though it omits the redirect handling code (which is also added in the following commit). Fixes dlang#166.
This implements support for redirects, which may be served by GitHub in some circumstances (such as when a repository is moved or renamed).
Avoid tasks getting stuck just because one request is stuck.
At this point, we do not yet know if this is a problem. expectOK should instead be used to convert non-2xx responses to exceptions.
Avoid another instance or thread from reading a partially-written file.
This fixes - `--cron-daily` mode eventually failing due to running out of file descriptors, because - performing repeated HTTP requests with Vibe.d causing a file descriptor leak because - Vibe.d kept erroneously dropping connections and reconnecting to the same HTTP server because - vibe.core.net.TCPConnection.waitForDataEx erroneously returning WaitForDataStatus.noMoreData instead of WaitForDataStatus.timeout in some situations because - (some unidentified bug in vibe-core fixed between 1.10.1 and 1.18.1). Update the compiler version to allow compiling the newer package as well.
- Capitalize footer, following the conventions of e.g. "Signed-off-by" - Ensure there is a blank line before the footer
By default, unit-threaded buffers and hides stdout/stderr output. This becomes a problem when an error is thrown in a unit test in a Vibe.d task. Normally, uncaught throwables are printed to stderr by a catch clause in vibe.core.task.TaskFiber.run, however, that output would be hidden by unit-threaded by default. Because vibe-core calls abort right after attempting to print the exception, we thus never get to see it. The solution is to use --debug in addition to --single, which disables output buffering in unit-threaded.
"prev" occurring before "next" was tripping up the old parser logic.
Issues are not pull requests, although the GitHub API (and UI) frequently lumps them together.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a batch of fixes for issues that I have encountered while moving the bot away from Heroku and onto a self-hosted system, and while fixing the daily cron job task (which has been dysfunctional for a while).
Please see the commit list and their messages for details.