-
Notifications
You must be signed in to change notification settings - Fork 67
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
Resolve Issue #277: Change Connection behavior to require both a packet send and receive #280
Resolve Issue #277: Change Connection behavior to require both a packet send and receive #280
Conversation
This changes connection events to only fire when a connection is considered established. Connections are considered established when we have both sent and received a packet from a particular destination. This ensures that the heartbeat behavior is consistent with the connection event behavior
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing and thanks for the contribution. It looks good to me! I will have to try it locally before merging it. One change I had some concerns about is the change of Timeout => Disconnect. This is a change users could be confused by. However, that should not stop us. We can do a major increment. Please have a look at some comments I made, those are minor comments
if self.is_established() { | ||
messenger.send_event( | ||
&self.remote_address, | ||
SocketEvent::Disconnect(self.remote_address), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this might bring in some confusion. Timeout is currently used for telling a connection timed out. Now, this even will represent a not established connection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I left Timeout (or rather create Disconnect
), was that I thought it was useful to have Timeout
as a concept even if a connection is never established.
For example, as a user if I send a Hello
to a server, and I never receive a reply, I need some way to be notified of that by Laminar. If we dropped Timeout
entirely from this PR, then there would be no way of seeing from Laminar
that we had no response.
So the semantics in this PR are that Timeout
is any timeout, whether connected or not, and Disconnect
is a timeout where we previously had a connection.
I'm also a little uncomfortable with the semantics here, and I'm happy to change things, but if we drop Timeout
, I think we need to have some way of notifying the user that we sent a Hello
, and never heard back.
PR feedback documentation changes. Co-authored-by: Timon <timonpost@hotmail.nl>
Based on PR feedback
I've incorporated all the documentation updates and dead-code removal. I'm also totally happy if we want to work through the |
Does anyone know why the |
@ncallaway It seems like the windows bare metal machine is down. It might be the case that amethyst has switched CI's I am not sure. |
So regarding the extra socket event. I am fine with it now, after reading your reasons. It seems like a useful thing to provide. If the user doesn't want to handle it it can just use |
@TimonPost I'm good with this being merged if the CI issue isn't a blocking problem. I'm also happy to wait until we resolve whatever the CI issue is, but I'm not sure I can be much assistance there. |
I;lll test it on windows then we can do a merge. |
Sorry for the delay, my week was filled with lots of other things. I tested it on windows, and it seems to behave good! Thanks for the contributions, lets merge. |
This PR is based on the discussion in #277.
This change:
SocketEvent::Connect
to only fire when a connection has been established.VirtualConnection
s from inbound messages, if we have too many unestablished connections already being tracked.max_unestablished_connections
.max_unestablished_connections
config option.SocketEvent::Disconnect
to fire when aTimeout
fires from an endpoint with an established connection.SocketEvent
s, and adds aConnection
section to the mdbook.Open Questions
Disconnect
SocketEvent. It seemed useful to be to both mirror theConnect
event, and to disambiguate aTimeout
from an established connection being dropped. Still, it's one moreSocketEvent
that users need to be aware of and manage. As an alternative, we could modifyTimeout
to include a flag to indicate whether it was an established connection that timed out. As a third option, we could just drop this event all together and require users to know which endpoints are established connections and which aren't.Connections
should have their own section in the book. I decided to make it it's own section, because it seems like this is a current area of confusion for Laminar users. I'm happy to make changes or document this somewhere else.