Bioconductor / BiocParallel

Bioconductor facilities for parallel evaluation
https://bioconductor.org/packages/BiocParallel
67 stars 29 forks source link

Check the port status before creating the cluster #187

Closed Jiefei-Wang closed 2 years ago

Jiefei-Wang commented 2 years ago

When the user does not specify the port number, SnowParam will randomly choose a port without verifying whether the port is used or not, so it might select a port that is being used by the other process. This pull request alleviates the problem by adding a port check before returning the random port number.

mtmorgan commented 2 years ago

I think this should be more informative to the user, e.g.,

message("failed to connect on port xxx, trying again...")

and after say five failures fail hard with with .stop("cannot find open port; see <relevant section in vignette or help pages>").

Jiefei-Wang commented 2 years ago

Thanks, do you mean we should give a message/warning when the user specifies the port number but the number is occupied? I feel that when the user uses the default setting, this should be a part of the port searching procedure and we do not have to get the user's attention unless there is any error. Please let me know your thought.

mtmorgan commented 2 years ago

In the patch I just saw an infinite loop? Which doesn't sound like a good idea... or did I misread the code?

Jiefei-Wang commented 2 years ago

I agree, I can remove the infinite loop and use the for loop instead. I just wonder about your first suggestion

I think this should be more informative to the user, e.g., message("failed to connect on port xxx, trying again...")

should we let the user know it when we are trying to search for the port? I think we should not print this message when the user is using the default.

mtmorgan commented 2 years ago

I think the message should be printed each time the port selection fails.

Jiefei-Wang commented 2 years ago

Thanks, I have changed it to a for loop and give a message when the port is not open.