Remove timeout used with NPM command

[TatuLund]

Addresses #6299

Supercedes #6165

---

This change is 

[ujoni]

After a year-long internal debate we have arrived to the conclusion that the timeout should be removed entirely. This is mostly to adhere to the semver nicely.

Adding a public constant and a configurable system property would essentially require +0.1.0 in the strictest sense. We can do a fancier fix for Flow 2.1 that allows for configuration.

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @TatuLund)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 127 at r1 (raw file):

        boolean commandExited = false;
        try {
            commandExited = process.waitFor(processTimeout, TimeUnit.SECONDS);

Please remove timeout entirely. Printing a log message, if the process took a long time could be nice. Say the command execution takes 5s+, we could log a message informing the user what took forever. Maybe that would enable user to investigate and potentially resolve problems with their system.

[ujoni]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @TatuLund)

---

flow-server/src/main/java/com/vaadin/flow/server/Constants.java, line 156 at r1 (raw file):

     * Configuration name for timeout used for NPM detection
     */
    public static final String NPM_COMMAND_TIMEOUT = "npmCommandTimeout";

Please remove

[TatuLund]

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @ujoni and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/Constants.java, line 153 at r1 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

First sentence of Javadoc is incomplete (period is missing) or not present.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/Constants.java, line 156 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Please remove

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 127 at r1 (raw file):

Previously, ujoni (Joni) wrote…

Please remove timeout entirely. Printing a log message, if the process took a long time could be nice. Say the command execution takes 5s+, we could log a message informing the user what took forever. Maybe that would enable user to investigate and potentially resolve problems with their system.

Done.

[ujoni]

Reviewed 2 of 2 files at r2.
Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @vaadin-bot)

[ujoni]

Reviewable status: 2 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @TatuLund and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 125 at r2 (raw file):

        long timeStamp = System.currentTimeMillis();
        try {
            commandExited = process.waitFor();

Ah, waitFor() returns an exit code instead of a boolean.

[TatuLund]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @ujoni and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 125 at r2 (raw file):

Previously, ujoni (Joni) wrote…

Ah, waitFor() returns an exit code instead of a boolean.

Changed logic accordingly

[TatuLund]

Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @ujoni and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 125 at r2 (raw file):

Previously, TatuLund (Tatu Lund) wrote…

Changed logic accordingly

Done.

[ujoni]

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @TatuLund, @ujoni, and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 122 at r3 (raw file):

        }

        int exitCode = 0;

Please initialize this to -1. All non-negative exit codes should be considered as "valid" in a sense that the process did exit on its own, and does not need to be force-closed.

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 132 at r3 (raw file):

            return Optional.empty();
        } finally {
            if (exitCode != 0) {

With the changed initial value, should be exitCode == -1.

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 146 at r3 (raw file):

        if (exitCode != 0) {
            log().error(
                    "Could not get a response from '{}' command",

This particular log can be moved inside the

finally {
    if (exitCode == -1)

and could be replaced with

if (exitCode > 0) {
    log().error("Command '{}' failed with exit code '{}'", commandString, exitCode);
}

[TatuLund]

Reviewable status: 5 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @ujoni and @vaadin-bot)

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 122 at r3 (raw file):

Previously, ujoni (Joni) wrote…

Please initialize this to -1. All non-negative exit codes should be considered as "valid" in a sense that the process did exit on its own, and does not need to be force-closed.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 132 at r3 (raw file):

Previously, ujoni (Joni) wrote…

With the changed initial value, should be exitCode == -1.

Done.

---

flow-server/src/main/java/com/vaadin/flow/server/frontend/FrontendToolsLocator.java, line 146 at r3 (raw file):

Previously, ujoni (Joni) wrote…

This particular log can be moved inside the

finally {
    if (exitCode == -1)

and could be replaced with

if (exitCode > 0) {
    log().error("Command '{}' failed with exit code '{}'", commandString, exitCode);
}

Done.

[ujoni]

Reviewed 1 of 1 files at r4.
Reviewable status: 4 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @ujoni and @vaadin-bot)

[ujoni]

Reviewable status: 1 unresolved discussion, 1 of 1 LGTMs obtained (waiting on @vaadin-bot)

[ujoni]

Dismissed @vaadin-bot from a discussion.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained

[caalador]

Reviewed 1 of 1 files at r6.
Reviewable status: complete! all discussions resolved, 1 of 1 LGTMs obtained, and 1 stale