ecki commented on code in PR #446: URL: https://github.com/apache/mina-sshd/pull/446#discussion_r1435255754
########## sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java: ########## @@ -151,24 +156,34 @@ public abstract class AbstractSession extends SessionHelper { protected final Map<KexProposalOption, String> clientProposal = new EnumMap<>(KexProposalOption.class); protected final Map<KexProposalOption, String> unmodClientProposal = Collections.unmodifiableMap(clientProposal); protected final Map<KexProposalOption, String> negotiationResult = new EnumMap<>(KexProposalOption.class); - protected final Map<KexProposalOption, String> unmodNegotiationResult = Collections.unmodifiableMap(negotiationResult); + protected final Map<KexProposalOption, String> unmodNegotiationResult = Collections + .unmodifiableMap(negotiationResult); protected KeyExchange kex; protected Boolean firstKexPacketFollows; protected boolean initialKexDone; + protected final AtomicBoolean strictKexSignalled = new AtomicBoolean(); + protected final AtomicInteger newKeysReceivedCount = new AtomicInteger(); + protected final AtomicInteger newKeysSentCount = new AtomicInteger(); + /** * Holds the current key exchange state. */ protected final AtomicReference<KexState> kexState = new AtomicReference<>(KexState.UNKNOWN); protected final AtomicReference<DefaultKeyExchangeFuture> kexFutureHolder = new AtomicReference<>(null); - // The kexInitializedFuture is fulfilled when this side (client or server) has prepared its own proposal. Access is + // The kexInitializedFuture is fulfilled when this side (client or server) + // has prepared its own proposal. Access is // synchronized on kexState. protected DefaultKeyExchangeFuture kexInitializedFuture; /* * SSH packets encoding / decoding support */ + /** Input packet sequence number. */ + protected long seqi; + /** Output packet sequence number. */ + protected long seqo; Review Comment: Unfortunatelly your patch does modify unrelated things, it would be good not to have to review them. it is relevant to the patch because you introduced a synchronized and it was not clear to me if it’s the right one (since it looks like the increase of those variables do not know about it). https://github.com/apache/mina-sshd/blob/f5c63a87d46ca5820e17af00b4e65a43288b58c7/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java#L1615 vs. NewKeysSignaled / reset yes it’s a complex code, but not documenting assumptions doesn’t make it less complex. Anyway, you don’t need to add the guard when you are sure it’s either correct or not needed. -- 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: dev-unsubscr...@mina.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org For additional commands, e-mail: dev-h...@mina.apache.org