initial memberlist.Transport implementation allowing us to use an upgrader for ListenTCP/ListenUDP #7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
amery
force-pushed
the
pr-amery-transport
branch
from
f298138
to
3535792
Compare
karasz
reviewed
Feb 15, 2023
karasz
reviewed
Feb 15, 2023
karasz
reviewed
Feb 15, 2023
transport/tcp.go
Outdated
| WithField(RemoteAddrLabel, conn.RemoteAddr()). | ||
| Print("Connection Terminated") | ||
|
|
||
| break |
There was a problem hiding this comment.
does this break do really what you want? Didn't you mean:
mainfor:for {
....
break mainfor
}There was a problem hiding this comment.
good catch, but what about using return instead? @karasz
karasz
reviewed
Feb 15, 2023
karasz
reviewed
Feb 15, 2023
karasz
reviewed
Feb 15, 2023
karasz
reviewed
Feb 15, 2023
transport/tcp.go
Outdated
| // continue | ||
| case <-ctx.Done(): | ||
| // sorry pal, we are cancelled | ||
| defer conn.Close() |
There was a problem hiding this comment.
defer in for is calling for trouble afaik
maybe
-defer conn.Close-
t.error(ctx.Err()).
WithField(ListenerAddrLabel, ln.Addr()).
WithField(RemoteAddrLabel, conn.RemoteAddr()).
Print("Connection Terminated")
+conn.Close()+
breakThere was a problem hiding this comment.
combined with return becomes more explicit that it only happens once
There was a problem hiding this comment.
This is still not fixed, line 60 is unchanged
There was a problem hiding this comment.
It's not in a loop because there is a return right after
karasz
requested changes
Feb 15, 2023
Signed-off-by: Alejandro Mery <[email protected]>
Signed-off-by: Alejandro Mery <[email protected]>
Signed-off-by: Alejandro Mery <[email protected]>
…ion before started Signed-off-by: Alejandro Mery <[email protected]>
Signed-off-by: Alejandro Mery <[email protected]>
amery
force-pushed
the
pr-amery-transport
branch
from
3535792
to
57aaec3
Compare
|
amended. also updated it to use the new slog/handlers/discard instead of slog/noop @karasz |
Signed-off-by: Alejandro Mery <[email protected]>
amery
force-pushed
the
pr-amery-transport
branch
from
57aaec3
to
519b520
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.