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

Client lifecycle diagram #567

Merged
merged 12 commits into from
Mar 10, 2021
Merged

Client lifecycle diagram #567

merged 12 commits into from
Mar 10, 2021

Conversation

Ethan-Arrowood
Copy link
Collaborator

@Ethan-Arrowood Ethan-Arrowood commented Feb 25, 2021

Closes #532

Opening as a draft to start getting some feedback. I've read through the source code, tests, and existing documentation to come up with this. It may not be correct + I feel like I want to expand the "processing" state since there is a lot that happens inside of that.

I'd love to integrate parts of the API here too like specific methods calls, emitted events, and property values

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won’t disconnect until keep alive timeout.

@Ethan-Arrowood
Copy link
Collaborator Author

It won’t disconnect until keep alive timeout.

Yes good point. I'll include some more details one that

@ronag
Copy link
Member

ronag commented Feb 25, 2021

I thinking you should have another state which is just connected.

Idle - pending - running - keep alive - closing - destroyed

@Ethan-Arrowood
Copy link
Collaborator Author

I thinking you should have another state which is just connected.
Idle - pending - running - keep alive - closing - destroyed

From what I can tell the Client never actually sits in a "connected" state. Once dispatch is called the Client immediately creates that socket connection and begins processing the request; even if it does not resolve the response back to the user immediately (as in the case of the async functions)

@codecov-io
Copy link

codecov-io commented Feb 25, 2021

Codecov Report

Merging #567 (372153d) into master (e35706c) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #567   +/-   ##
=======================================
  Coverage   99.16%   99.16%           
=======================================
  Files          16       16           
  Lines        1444     1444           
=======================================
  Hits         1432     1432           
  Misses         12       12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e35706c...372153d. Read the comment docs.

docs/api-lifecycle.md Outdated Show resolved Hide resolved
@Ethan-Arrowood
Copy link
Collaborator Author

Ethan-Arrowood commented Feb 26, 2021

Edit: This is an old diagram - up to date image is in the commit

@ronag what do you think about this breakdown of the processing state?

@ronag
Copy link
Member

ronag commented Feb 26, 2021

@Ethan-Arrowood maybe we could have a voice call? How does your day look today?

@Ethan-Arrowood
Copy link
Collaborator Author

Ethan-Arrowood commented Feb 26, 2021

Im free for the next couple hours! Send me an invite for whenever [edit: removed email]

@ronag
Copy link
Member

ronag commented Feb 27, 2021

We had a call and discussed the life cycle and also other ways to make the code easier to understand.

@Ethan-Arrowood
Copy link
Collaborator Author

Updated the diagram based on our conversation @ronag - I'm working on the detailed state descriptions now so its better understood as well

@Ethan-Arrowood Ethan-Arrowood marked this pull request as ready for review March 3, 2021 21:58
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the processing sub-diagram, you need a keep-alive state that goes back to the beginning or times out.

@Ethan-Arrowood
Copy link
Collaborator Author

In the processing sub-diagram, you need a keep-alive state that goes back to the beginning or times out.

IMO this is what the pending state represents, connected but not processing requests. The keepAlive event is what transitions the Client from processing back to pending. And when that socket connection timesout, the timeOut event transitions from pending to init.

@mcollina
Copy link
Member

mcollina commented Mar 4, 2021

I got confused by having both done and keep alive ending in the same dot.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

docs/api-lifecycle.md Outdated Show resolved Hide resolved
@Ethan-Arrowood
Copy link
Collaborator Author

@nodejs/undici please review and merge at your earliest convenience

@Ethan-Arrowood
Copy link
Collaborator Author

Just rebased and moved the file to the docsify site 😄

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still lgtm

@Ethan-Arrowood
Copy link
Collaborator Author

Okay now i think this is good to go. I messed things up with docsify links in the initial site build. But this should clean up everything

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag ronag merged commit ddcab20 into nodejs:master Mar 10, 2021
Ethan-Arrowood added a commit to Ethan-Arrowood/undici that referenced this pull request Mar 15, 2021
* add init state machine paragraph and diagram

* start extrapolating on processing state

* modify state diagram with feedback

* change svg to png for github dark mode

* update link

* adding details

* wrap up state machine explanation

* add missing period

* add lifecycle docs to site

* fix image link

* fix more broken links

* more broken links
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
* add init state machine paragraph and diagram

* start extrapolating on processing state

* modify state diagram with feedback

* change svg to png for github dark mode

* update link

* adding details

* wrap up state machine explanation

* add missing period

* add lifecycle docs to site

* fix image link

* fix more broken links

* more broken links
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Lifecycle Diagram
5 participants