Skip to content

Commit

Permalink
[fix][proxy] Only go to connecting state once (apache#19331)
Browse files Browse the repository at this point in the history
Relates to: apache#17831 (comment)

### Motivation

When the `ProxyConnection` handles a `Connect` command, that is the time to go to `Connecting` state. There is no other time that makes sense to switch to connecting. The current logic will go to connecting in certain re-authentication scenarios, but those are incorrect. By moving the state change to earlier in the logic, we make the state transition clearer and prevent corrupted state.

### Modifications

* Remove `state = State.Connecting` from the `doAuthentication` method, which is called multiple times for various reasons
* Add `state = State.Connecting` to the start of the `handleConnect` method.

### Verifying this change

The existing tests will verify this change, and reading through the code makes it clear this is a correct change.

### Does this pull request potentially affect one of the following parts:

Not a breaking change.

### Documentation

- [x] `doc-not-needed` 

It would be nice to map out the state transitions for our connection classes. That is our of the scope of this small improvement.

### Matching PR in forked repository

PR in forked repository: michaeljmarshall#21
  • Loading branch information
michaeljmarshall authored Jan 31, 2023
1 parent 17c58a5 commit c8650ce
Showing 1 changed file with 1 addition and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -447,12 +447,12 @@ private void doAuthentication(AuthData clientData)
LOG.debug("[{}] Authentication in progress client by method {}.",
remoteAddress, authMethod);
}
state = State.Connecting;
}

@Override
protected void handleConnect(CommandConnect connect) {
checkArgument(state == State.Init);
state = State.Connecting;
this.setRemoteEndpointProtocolVersion(connect.getProtocolVersion());
this.hasProxyToBrokerUrl = connect.hasProxyToBrokerUrl();
this.protocolVersionToAdvertise = getProtocolVersionToAdvertise(connect);
Expand Down

0 comments on commit c8650ce

Please sign in to comment.