Closed
Conversation
`Curl_hyper_stream` needs to distinguish between two kinds of `HYPER_TASK_EMPTY` tasks: (a) the `foreach` tasks it creates itself, and (b) background tasks that hyper produces. It does this by recording the address of any `foreach` task in `hyptransfer->endtask` before pushing it into the executor, and then comparing that against the address of tasks later polled out of the executor. This works right now, but there is no guarantee from hyper that the addresses are stable. `hyper_executor_push` says "The executor takes ownership of the task, which should not be accessed again unless returned back to the user with `hyper_executor_poll`". That wording is a bit ambiguous but with my Rust programmer's hat on I read it as meaning the task returned with `hyper_executor_poll` may be conceptually the same as a task that was pushed, but that there are no other guarantees and comparing addresses is a bad idea. This commit instead uses `hyper_task_set_userdata` to mark the `foreach` task with a `USERDATA_RESP_BODY` value which can then be checked for, removing the need for `hyptransfer->endtask`. This makes the code look more like that hyper C API examples, which use userdata for every task and never look at task addresses.
bagder
approved these changes
Sep 3, 2023
Member
|
Thanks! |
ptitSeb
pushed a commit
to wasix-org/curl
that referenced
this pull request
Sep 25, 2023
`Curl_hyper_stream` needs to distinguish between two kinds of `HYPER_TASK_EMPTY` tasks: (a) the `foreach` tasks it creates itself, and (b) background tasks that hyper produces. It does this by recording the address of any `foreach` task in `hyptransfer->endtask` before pushing it into the executor, and then comparing that against the address of tasks later polled out of the executor. This works right now, but there is no guarantee from hyper that the addresses are stable. `hyper_executor_push` says "The executor takes ownership of the task, which should not be accessed again unless returned back to the user with `hyper_executor_poll`". That wording is a bit ambiguous but with my Rust programmer's hat on I read it as meaning the task returned with `hyper_executor_poll` may be conceptually the same as a task that was pushed, but that there are no other guarantees and comparing addresses is a bad idea. This commit instead uses `hyper_task_set_userdata` to mark the `foreach` task with a `USERDATA_RESP_BODY` value which can then be checked for, removing the need for `hyptransfer->endtask`. This makes the code look more like that hyper C API examples, which use userdata for every task and never look at task addresses. Closes curl#11779
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
2 participants
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.
Curl_hyper_streamneeds to distinguish between two kinds ofHYPER_TASK_EMPTYtasks: (a) theforeachtasks it creates itself, and (b) background tasks that hyper produces. It does this by recording the address of anyforeachtask inhyptransfer->endtaskbefore pushing it into the executor, and then comparing that against the address of tasks later polled out of the executor.This works right now, but there is no guarantee from hyper that the addresses are stable.
hyper_executor_pushsays "The executor takes ownership of the task, which should not be accessed again unless returned back to the user withhyper_executor_poll". That wording is a bit ambiguous but with my Rust programmer's hat on I read it as meaning the task returned withhyper_executor_pollmay be conceptually the same as a task that was pushed, but that there are no other guarantees and comparing addresses is a bad idea.This commit instead uses
hyper_task_set_userdatato mark theforeachtask with aUSERDATA_RESP_BODYvalue which can then be checked for, removing the need forhyptransfer->endtask. This makes the code look more like that hyper C API examples, which use userdata for every task and never look at task addresses.