Introduce context to golang APIs#7719
Open
dentiny wants to merge 14 commits into
Open
Conversation
Member
|
Thank you for your hard work bringing the Go binding into the real world! I'm fine with the Go bindings, but I'd like to hear the members' views on the C part @apache/opendal-committers |
This file contains hidden or 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
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.
Which issue does this PR close?
Closes #7608
Rationale for this change
Using
contextfor IO operations is the general practice in golang; I cannot push forward adoption without this.What changes are included in this PR?
This PR adds context to all golang interfaces.
High-level designs
Design principle:
contextpassed as the first argument; when context timeout or cancels, underlying IO operations should be cancelled in a blocking manner, without actual IO operation completesThere're a key changes:
selectstatement subscribing both completed IO operation signal, and finished contextTesting and code review
Consideration this is a huge PR, I did a few things to improve correctness guarantee:
Key code reference
How golang side handles IO and context finish
https://github.com/dentiny/opendal/blob/f9d3a2f877cdc41f71efca8631397a7ea516ebb7/bindings/go/ffi.go#L44-L93
How C binding handles IO and cancellation
https://github.com/dentiny/opendal/blob/f9d3a2f877cdc41f71efca8631397a7ea516ebb7/bindings/c/src/cancel.rs#L145-L159
How blocking IO is implemented
https://github.com/dentiny/opendal/blob/f9d3a2f877cdc41f71efca8631397a7ea516ebb7/bindings/c/src/cancel.rs#L165-L178
PR split considerations
Since it's a large PR, I considered to split it into smaller piece
Option-1: add C-binding part first, then go binding
My thought: viable, but C-binding alone is not straightforward enough to tell what implementation plan I'm doing; and two 2K LOC PR is not much easier to review than one 4K LOC PR
Option-2: implement one APIs one by one, for example, do read and core change first, then write, list, presign.
My thought: viable, but the biggest review difficulty still comes from the first large PR; if reviewers think it better I'm willing to split the PR into smaller pieces for easier review.
Are there any user-facing changes?
Yes
AI Usage Statement
Plan mode with GPT-5.5, with milestones broken down, and opus-4.7 for implementation