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

TCP state machine diagram and description #95

Closed
wants to merge 13 commits into from

Conversation

guybedford
Copy link
Contributor

I've written up my own understanding of the TCP state machine so far as I've best been able to interpret it for implementation.

This provides a formal definition of all states and intermediate states, along with the pollable state in each of these.

This is what I'm going ahead with in the JCO implementation. If there are any inaccuracies I would be very grateful to know.

@guybedford
Copy link
Contributor Author

guybedford commented Jan 16, 2024

@guybedford
Copy link
Contributor Author

guybedford commented Jan 16, 2024

Note also in this interpretation I've specified both invalid-state errors as recoverable and also async access denied errors as recoverable based on reverting back into the previous state before the start call.

@badeend
Copy link
Collaborator

badeend commented Jan 17, 2024

Awesome, this helps a lot!

This looks about 90% right to me. Some remarks:

  • "finishBind() error" goes from TCP_BIND_READY to TCP_ERROR. This doesn't seem correct. Binds can be retried.
  • You've modeled TCP_ERROR and TCP_CLOSED as two dinstinct end-states. Why is that?
  • An error from startListen reverts back to TCP_BOUND, but a (non-would-block) error from finishListen terminates in TCP_ERROR. In general, there shouldn't any discrepencies in which state start-* errors and finish-* error end up. Because in preview3+ this distinction won't exist anymore. Which is why I changed this (very recently)
    in Doc updates #92
  • You reference the terms "granted" and "denied" a lot, but I don't see how permissions are relevant to the state machine. I think these terms could be generalized to Success and Failure, where access-denied is just one of the error codes.

I'm not so sure about codifying the separation between:

  • TCP_BIND & TCP_BIND_READY
  • TCP_CONNECT & TCP_CONNECT_READY
  • TCP_LISTEN & TCP_LISTEN_READY
    I can understand that JCO decided to implement it this way, but the distinction between these two shouldn't be observable from the consumer's POV, right? In my mind, they're more a concern of the pollable's implementation, rather than the socket state.

The TCP_CONNECT_READY and TCP_LISTEN_READY states should eagerly handle socket connection and socket listen calls respectively, so that the finish calls represent completion of the asynchronous operation. Implementations may perform blocking connect and listen in the connectFinish and listenFinish calls, but this is discouraged.

I don't understand what you mean with this. Implementations may definitely not block on connect and POSIX listen never blocks.


I've updated the diagram and documentation over here: badeend@beef70f. Changes:

  • updated the terminology to match the existing proposal.
  • incorporated my remarks from above
  • decoupled the socket states from the pollable logic.
  • integrated the state definitions into the WITs themselves.
  • added some clarifications from Async behaviour clarifications #89 into the WITs
  • these changes got quite bulky, so I moved it into its own file.

@guybedford
Copy link
Contributor Author

Thanks I actually just pushed an update in 2f39db4 that corrects the first remark bullet point of yours, which I also just noticed this morning.

The update corrects the _READY state handling as you mention, but the reason I modelled these states was specifically to be able to have a model of the poll state machine that can directly map to the TCP state machine. With the one describable exception then only being when in the LISTENING state that the pollable corresponds to the socket IO, but in all other states the poll state is simply a binary decision function based on the socket state - IF we add the _READY states.

Because the pollables was my biggest point of misunderstanding and confusion (costing me a week in implementation time), this was my primary goal to ensure these states were fully described comprehensively and without any ambiguity.

I don't understand what you mean with this. Implementations may definitely not block on connect and POSIX listen never blocks.

We initially implemented the bind and listen calls in the bindFinish and listenFinish methods. I was attempting to clarify that while this is possible, it is not the design. Again, trying to clarify a confusion we had that cost us development time.

Your changes look great otherwise - I do think adding the _READY states for the polls would be preferable though, and have submitted a PR to your change in badeend#1.

Lastly, I also implemented the permission denied errors as recoverable, in that one could imagine later granting the permission on the system and then trying the operation again on the socket. I'm open to whatever here, but I think it would be good to specify this behaviour.

@guybedford
Copy link
Contributor Author

After further review, the updates in badeend@beef70f seem like they do cover everything here to me.

The only remaining clarification to still be answered that I have then is #96.

@guybedford guybedford closed this Jan 17, 2024
@badeend
Copy link
Collaborator

badeend commented Jan 17, 2024

Implementations may definitely not block on connect (...)

We initially implemented the bind and listen calls in the bindFinish and listenFinish methods. I was attempting to clarify that while this is possible, it is not the design. Again, trying to clarify a confusion we had that cost us development time.

In POSIX, bind and listen never block, so calling them in the finish-* function is perfectly fine and even suggested in the readme. My remark about blocking was specifically on connect.

I've appended notes to my branch above to clarify this for future readers.

This was referenced Jan 17, 2024
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.

2 participants