lgoldstein commented on a change in pull request #201: URL: https://github.com/apache/mina-sshd/pull/201#discussion_r670119533
########## File path: sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java ########## @@ -109,6 +109,16 @@ */ public static final String SESSION = "org.apache.sshd.session"; + /** + * A last-resort timeout for waiting after having received a KEX_INIT message from the peer until we have prepared + * our own KEX proposal. This timeout should actually never be hit unless there is a serious deadlock somewhere and + * the session is never closed. + * + * @see <a href="https://issues.apache.org/jira/browse/SSHD-1197">SSHD-1197</a> + * @see #doKexNegotiation() + */ + private static final Duration KEX_PROPOSAL_SETUP_TIMEOUT = Duration.ofSeconds(42); Review comment: >>> If we make that configurable, users can very easily shoot themselves in the foot by setting a (very) low value, like 0. I understand your concern, but personally I believe in total user responsibility - I don't like being told by someone that "they know better", so I am reluctant to impose my opinion on others (BTW, this is also why I always insist that all our internal variables and/or methods be accessible either via getters or as public/protected members). In this context, the same could be said for *all* our configuration values - users can shoot themselves in the foot by choosing a wrong or conflicting value. However, I believe this is a good thing - they will learn... IMO, we are not supposed to protect our users from mistakes that occur because they do not bother to read the documentation or understand the consequences of their actions. In this case, perhaps we could do something that would satisfy both our concerns: ```java Duration waitTime = CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this); ValidateUtils.checkTrue(waitTime.toMillis() > 0L, "Infinite/Undefined value N/A: %d", waitTime); ``` >> Lengthening this should never be necessary. The thread that is preparing this side's proposal is already running. While it seems reasonable, we don't know what weird usages our users might encounter or what unforeseen circumstances. Therefore, I believe we should give them the flexibility to decide for themselves. Bottom line - I trust your judgement, and if you still feel that a constant value is more appropriate I will not object to it. In any case, let's also update the README (perhaps add a KEX section) and explain this mechanism so that if there are future questions or suspicions of bugs around this we will have a clear picture of the implementation and the choice of either a constant or configurable timeout value. -- 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