hubcio commented on code in PR #3652:
URL: https://github.com/apache/iggy/pull/3652#discussion_r3568472401
##########
foreign/go/client/tcp/tcp_core.go:
##########
@@ -644,7 +652,8 @@ func (c *IggyTcpClient) shutdown() error {
}
}
- c.state = iggcon.StateShutdown
+ c.transportState = iggcon.TransportStateShutdown
+ c.sessionState = iggcon.SessionStateUnauthenticated
Review Comment:
pre-existing ordering, but this line extends it to `sessionState`: if
`conn.Close()` errors, the early return above skips both state flips, leaving
the client stuck reporting connected (and now authenticated too).
`disconnect()` does it in the safer order - state first, then close. worth
mirroring that here, or just not early-returning before the flips.
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -30,41 +30,42 @@ import (
)
func (c *IggyTcpClient) LoginUser(ctx context.Context, username string,
password string) (*iggcon.IdentityInfo, error) {
- c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
- buffer, err := c.do(ctx, &command.LoginUser{
+ return c.login(ctx, &command.LoginUser{
Username: username,
Password: password,
})
- if err != nil {
- return nil, err
- }
-
- c.logger.Info("Iggy client has signed in successfully.",
slog.String("client_address", c.clientAddress))
- identity := binaryserialization.DeserializeLogInResponse(buffer)
- shouldRedirect, err := c.HandleLeaderRedirection(ctx)
- if err != nil {
- return nil, err
- }
- if shouldRedirect {
- if err = c.Connect(ctx); err != nil {
- return nil, err
- }
- return c.LoginUser(ctx, username, password)
- }
- return identity, nil
}
func (c *IggyTcpClient) LoginWithPersonalAccessToken(ctx context.Context,
token string) (*iggcon.IdentityInfo, error) {
- c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
- buffer, err := c.do(ctx, &command.LoginWithPersonalAccessToken{
+ return c.login(ctx, &command.LoginWithPersonalAccessToken{
Token: token,
})
+}
+
+func (c *IggyTcpClient) login(ctx context.Context, loginCmd command.Command)
(*iggcon.IdentityInfo, error) {
+ c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
+ c.mtx.Lock()
+ pre := c.sessionState
+ c.sessionState = iggcon.SessionStateAuthenticating
+ c.mtx.Unlock()
+
+ buffer, err := c.do(ctx, loginCmd)
if err != nil {
+ // A rejected login leaves the session state untouched, so
restore the
+ // pre-login state only while it is still Authenticating: if
the state
+ // moved, the connection may died mid-attempt and invalidation
already
+ // recorded it unauthenticated, skip the restoration.
+ c.mtx.Lock()
+ if c.sessionState == iggcon.SessionStateAuthenticating {
Review Comment:
there's a race here when two goroutines call `LoginUser` concurrently on the
same client. the mutex is dropped between setting `Authenticating` (line 49)
and this restore check, and `do()` takes its own lock, so two logins can
interleave: A's connection dies mid-login (`invalidateConnLocked` sets
`Unauthenticated`), B starts a login and sets `Authenticating` again, A
re-locks, sees `Authenticating` and restores its stale `pre` = `Authenticated`.
B then fails the disconnected gate and skips its own restore. end state:
`sessionState = Authenticated` while `transportState = Disconnected`, both
logins returned errors.
nothing reads `sessionState` yet so this can't bite today, but #3650 will
add exactly that reader (deciding whether to re-login on reconnect), so better
to fix it before the machinery grows a consumer.
simplest fix: drop the `Authenticating` intermediate and the `pre`/restore
entirely - set `Authenticated` on success only, leave `sessionState` untouched
on failure. connection death already resets it via `invalidateConnLocked`, and
a server-side reject never touched it in the first place, so behavior is
identical on all paths and `TestLoginUser_RejectedReloginKeepsExistingSession`
still passes. ~10 lines less and no race. if #3650 needs an observable
`Authenticating`, it can be re-added then together with real login
serialization (an in-progress guard held across the whole attempt) - the
current best-effort restore doesn't provide that anyway.
##########
foreign/go/contracts/state.go:
##########
@@ -17,30 +17,45 @@
package iggcon
-type State uint8
+type TransportState uint8
const (
- StateShutdown State = iota
- StateDisconnected
- StateConnecting
- StateConnected
- StateAuthenticating
- StateAuthenticated
+ TransportStateDisconnected TransportState = iota
Review Comment:
removing the exported `State` type and consts is a compile-time break for
anyone importing `iggcon` directly - deserves a breaking-change note and a
minor (not patch) bump for the next tag. also the zero value changed meaning:
`StateShutdown` was 0, now `TransportStateDisconnected` is 0, so a zero-value
client is connectable instead of permanently shut down. arguably better, just
worth a mention in the note.
##########
foreign/go/client/tcp/tcp_session_management.go:
##########
@@ -30,41 +30,42 @@ import (
)
func (c *IggyTcpClient) LoginUser(ctx context.Context, username string,
password string) (*iggcon.IdentityInfo, error) {
- c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
- buffer, err := c.do(ctx, &command.LoginUser{
+ return c.login(ctx, &command.LoginUser{
Username: username,
Password: password,
})
- if err != nil {
- return nil, err
- }
-
- c.logger.Info("Iggy client has signed in successfully.",
slog.String("client_address", c.clientAddress))
- identity := binaryserialization.DeserializeLogInResponse(buffer)
- shouldRedirect, err := c.HandleLeaderRedirection(ctx)
- if err != nil {
- return nil, err
- }
- if shouldRedirect {
- if err = c.Connect(ctx); err != nil {
- return nil, err
- }
- return c.LoginUser(ctx, username, password)
- }
- return identity, nil
}
func (c *IggyTcpClient) LoginWithPersonalAccessToken(ctx context.Context,
token string) (*iggcon.IdentityInfo, error) {
- c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
- buffer, err := c.do(ctx, &command.LoginWithPersonalAccessToken{
+ return c.login(ctx, &command.LoginWithPersonalAccessToken{
Token: token,
})
+}
+
+func (c *IggyTcpClient) login(ctx context.Context, loginCmd command.Command)
(*iggcon.IdentityInfo, error) {
+ c.logger.Info("Iggy client is signing in...",
slog.String("client_address", c.clientAddress))
+ c.mtx.Lock()
+ pre := c.sessionState
+ c.sessionState = iggcon.SessionStateAuthenticating
+ c.mtx.Unlock()
+
+ buffer, err := c.do(ctx, loginCmd)
if err != nil {
+ // A rejected login leaves the session state untouched, so
restore the
+ // pre-login state only while it is still Authenticating: if
the state
+ // moved, the connection may died mid-attempt and invalidation
already
Review Comment:
typo: "the connection may died" -> "may have died"
##########
foreign/go/client/tcp/tcp_core_test.go:
##########
@@ -272,3 +273,23 @@ func TestNewIggyTcpClient_StoresProvidedLogger(t
*testing.T) {
t.Errorf("expected logger output to contain 'source=tcp', got:
%q", output)
}
}
+
+func TestLoginUser_RejectedReloginKeepsExistingSession(t *testing.T) {
Review Comment:
two more asserts would pin the happy paths while you're here: login success
sets `Authenticated`, and `LogoutUser` success resets to `Unauthenticated` -
right now no test covers either transition.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
