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

Remove hyptransfer->endtask. #11779

Closed
wants to merge 1 commit into from
Closed

Conversation

nnethercote
Copy link
Contributor

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.

`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.
@github-actions github-actions bot added the Hyper label Sep 1, 2023
@bagder bagder closed this in 50aa325 Sep 3, 2023
@bagder
Copy link
Member

bagder commented Sep 3, 2023

Thanks!

@nnethercote nnethercote deleted the rm-endtask branch September 3, 2023 21:22
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants