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

Refactor OneDriveApi implementation #2608

Conversation

JC-comp
Copy link

@JC-comp JC-comp commented Jan 28, 2024

This pull request resolves the OneDriveAPI refactoring part of #2530.

Changes

  • Add error handling wrapper for all request to handle retry automatically
  • Record the request and response for all requests to facilitate dumping information when errors occur, eliminating the need to reproduce them again with a higher log level
  • Replace the use of force exit with throwing errors, allowing the caller to handle them to prevent process loss
  • To prevent out-of-order execution caused by acquiring the auth token, all connection information, request headers, and callbacks of the Curl socket have been delayed and set altogether

Notes

What is still missing?

  • Refactor callers in Sync.d is pending until the merging and settling of all related tool refactorings
  • Replace the force exit operation in the authentication flow
  • Beautify logging format

src/util.d Show resolved Hide resolved
@abraunegg abraunegg added this to the v2.5.0 milestone Feb 11, 2024
@abraunegg
Copy link
Owner

abraunegg commented Feb 11, 2024

@JC-comp
This PR fails to compile:

ldc2  -w -g -O -J. -d-debug -gc -L-lcurl -L-lsqlite3  -L-ldl src/main.d src/config.d src/log.d src/util.d src/qxor.d src/curlEngine.d src/onedrive.d src/webhook.d src/sync.d src/itemdb.d src/sqlite.d src/clientSideFiltering.d src/monitor.d src/arsd/cgi.d -ofonedrive
src/arsd/cgi.d(3921): Error: static assert:  "Not implemented in your compiler version!"
src/webhook.d(112):        instantiated from here: serveEmbeddedHttp!(handle, Cgi, 5000000L)

So in your testing and building, you must test each PR separately, by itself, compiling against the minimum LDC version, using a script similar to the following (assuming you already know how to install 'ldc' manually, and can install v1.20.1):

#!/bin/bash
  
PR=2608

rm -rf ./onedrive-pr${PR}
git clone https://github.com/abraunegg/onedrive.git onedrive-pr${PR}
cd onedrive-pr${PR}
git fetch origin pull/${PR}/head:pr${PR}
git checkout pr${PR}

# MIN LDC Version to compile
# MIN Version for ARM / Compiling with LDC
#source ~/dlang/ldc-1.18.0/activate
source ~/dlang/ldc-1.20.1/activate


./configure --enable-debug --enable-notifications; make clean; make;
deactivate
./onedrive --version

Post building, then start running all your tests for which the PR is resolving and/or fixing.

@JC-comp
Copy link
Author

JC-comp commented Feb 12, 2024

Update

  • Merge changes to ensure backward compatibility with ldc v1.20.1.

@abraunegg
Copy link
Owner

@JC-comp
With this PR, please can you:

  • Fix the current conflicts
  • Baseline re-merge the current 'alpha-5' into this PR, so that this can be tested as a complete item

@JC-comp
Copy link
Author

JC-comp commented Mar 8, 2024

@abraunegg
'alpha-5' re-based

@abraunegg
Copy link
Owner

@JC-comp
Please re-fix merge issues

Additionally, this PR breaks the entire fetching the correct re-try value and sets it to a hard coded 300 second value. Please restore original functionality.

@JC-comp
Copy link
Author

JC-comp commented Mar 8, 2024

@abraunegg
Conflicts fixed.

For the hard-coded retry value, it's a redundant function slated for removal in the future sync.d refactoring work. It will never be called in this version, as all retry mechanisms are handled at the OneDrive API level.

onedrive/src/onedrive.d

Lines 1311 to 1318 in c8fc1e6

case 429:
// If OneDrive sends a status code 429 then this function will be used to process the Retry-After response header which contains the value by which we need to wait
addLogEntry("Handling a OneDrive HTTP 429 Response Code (Too Many Requests)");
// Read in the Retry-After HTTP header as set and delay as per this value before retrying the request
thisBackOffInterval = response.getRetryAfterValue();
addLogEntry("Using Retry-After Value = " ~ to!string(thisBackOffInterval), ["debug"]);
break;

@abraunegg
Copy link
Owner

@JC-comp
For this PR to proceed any further, the following needs to be fixed:

  1. Coding style - function declarations are not consistent with the coding style
  2. Language in changed logging output needs to be re-instated
  3. Output logging as per original functionality needs to be re-instated
  4. Example of how this updated code should be used to change all the error handling in sync.d and where required
  5. All error handling needs to be handed as per before - there are highly specific parts in the code where by the response of X does Y but in other parts does Z for the same response from the API ...

Overall the code 'works' but this is IMHO only partially complete. I am not going to consider this for v2.5.0 at this stage as this needs a lot more work.

@abraunegg abraunegg modified the milestones: v2.5.0, v2.5.x Mar 10, 2024
@JC-comp
Copy link
Author

JC-comp commented Mar 10, 2024

@abraunegg
I'm uncertain what I can do to push this PR further.

1-3 involve coding style and output formatting preferences, which lack formal documentation for strict adherence.

4, for the sync.d refactoring, the essential change introduced by this PR is the removal of the retry mechanism, which is already addressed at the API level, and ensuring appropriate actions are taken when the retry limit is reached.

5 seems ambiguous to me. Aside from implementing a back-off mechanism at higher level, all errors are propagated back to the original calling function. If certain responses trigger different actions, it might suggest a flaw in the design of the sync.d functions.

As this is initial step toward addressing potential bugs and unhandled error cases in sync.d, without this PR, I cannot proceed with the subsequent tasks.

abraunegg added a commit that referenced this pull request Apr 12, 2024
* Refine code changes from #2608 based on memory allocation and usage
abraunegg added a commit that referenced this pull request Apr 24, 2024
* Rework entire #2608 due to errors, memory leaks and handling
* Fix 429 'retry-after' header searching to use actual value, not 120 all the time
* General cleanup of code from #2608
@abraunegg
Copy link
Owner

Closing in favour of #2686

@abraunegg abraunegg closed this Apr 24, 2024
@JC-comp JC-comp deleted the webhook_refactor_to_api branch April 30, 2024 05:57
@abraunegg
Copy link
Owner

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

Repository owner locked and limited conversation to collaborators May 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants