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

P2P, HTTP, SHiP: Change fc::create_listener to use strand #819

Closed
heifner opened this issue Sep 24, 2024 · 2 comments · Fixed by #909
Closed

P2P, HTTP, SHiP: Change fc::create_listener to use strand #819

heifner opened this issue Sep 24, 2024 · 2 comments · Fixed by #909
Assignees
Labels
bug The product is not working as was intended. 👍 lgtm OCI Work exclusive to OCI team tech-debt

Comments

@heifner
Copy link
Member

heifner commented Sep 24, 2024

Currently fc::create_listener creates sockets with the provided executor. For our use cases, it would be nice if fc::create_listener would create a strand for each socket. The users of fc::create_listener could then retrieve the strand from the socket.get_executor() for use.

Something like:
438ae62

Then:

  • Update SHiP to use that strand via socket.get_executor() instead of making a new strand.
  • Update net_plugin to use the socket strand instead of creating a different one.
@spoonincode
Copy link
Member

create_listener shouldn't blanket do this because some users of it (http) don't actually need the per connection strand. It should be driven by a parameter or different function name

@spoonincode
Copy link
Member

Actually, because of #816 (comment) I'm not sure create_listener should simply make the strand on the same executor it's using for listening. For this particular case it really seems best for the listener to remain on the main thread and have the socket with a strand in ship's thread. So maybe create_listener needs to take a functor that is called on each async_accept() that returns the executor for that call. To provide the most flexibility.

@arhag arhag added bug The product is not working as was intended. and removed triage tech-debt labels Sep 30, 2024
@arhag arhag added this to the Spring v1.0.3 milestone Sep 30, 2024
@heifner heifner self-assigned this Oct 1, 2024
@heifner heifner added the OCI Work exclusive to OCI team label Oct 1, 2024
@heifner heifner moved this from Todo to In Progress in Team Backlog Oct 1, 2024
@heifner heifner removed their assignment Oct 7, 2024
@heifner heifner removed the OCI Work exclusive to OCI team label Oct 7, 2024
@heifner heifner moved this from In Progress to Todo in Team Backlog Oct 7, 2024
@heifner heifner self-assigned this Oct 7, 2024
@heifner heifner added the OCI Work exclusive to OCI team label Oct 7, 2024
@heifner heifner moved this from Todo to In Progress in Team Backlog Oct 7, 2024
@arhag arhag added the 👍 lgtm label Oct 7, 2024
heifner added a commit that referenced this issue Oct 8, 2024
@heifner heifner linked a pull request Oct 8, 2024 that will close this issue
@heifner heifner removed this from the Spring v1.0.3 milestone Oct 8, 2024
@heifner heifner added this to the Spring v1.1.0-rc1 milestone Oct 8, 2024
heifner added a commit that referenced this issue Oct 8, 2024
heifner added a commit that referenced this issue Oct 8, 2024
heifner added a commit that referenced this issue Oct 8, 2024
heifner added a commit that referenced this issue Oct 8, 2024
@github-project-automation github-project-automation bot moved this from In Progress to Done in Team Backlog Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The product is not working as was intended. 👍 lgtm OCI Work exclusive to OCI team tech-debt
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants