Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent behaviour of dialyzer with ssh_sftp:start_channel/2 #9359

Open
alexandrejbr opened this issue Jan 29, 2025 · 0 comments
Open

Inconsistent behaviour of dialyzer with ssh_sftp:start_channel/2 #9359

alexandrejbr opened this issue Jan 29, 2025 · 0 comments
Assignees
Labels
bug Issue is reported as a bug team:PS Assigned to OTP team PS

Comments

@alexandrejbr
Copy link
Contributor

Describe the bug
Passing connect_timeout as an option to ssh_sftp:start_channel/2 gives the following dialyzer warning:

src/mylib_app.erl
Line 12 Column 1: Function start/2 has no local return
Line 14 Column 5: The contract ssh_sftp:start_channel(ssh:open_socket(),[ssh:client_option() | sftp_option()]) -> {'ok',pid(),ssh:connection_ref()} | {'error',reason()}
    ; (ssh:connection_ref(),[sftp_option()]) -> {'ok',pid()} | {'ok',pid(),ssh:connection_ref()} | {'error',reason()}
    ; (ssh:host(),[ssh:client_option() | sftp_option()]) -> {'ok',pid(),ssh:connection_ref()} | {'error',reason()} cannot be right because the inferred return for start_channel(Pid::pid(),[{'connect_timeout', 1000}]) on line {14,5} is any()

To Reproduce
Analyse with dialyzer the following code (make sure to include the ssh application, I used a rebar3 app where I changed the app.src file)

f() ->
    {ok, Pid} = ssh:connect("127.0.0.1", 22, []),
    ssh_sftp:start_channel(Pid, [{connect_timeout, 1000}]),
   ok.

Expected behavior
No dialyzer warning should be shown. That being said, if you look at the implementation you can see that TimeOut can be returned:

-spec start_channel(ssh:open_socket(),
                    [ssh:client_option() | sftp_option()]
                   )
                   -> {ok,pid(),ssh:connection_ref()} | {error,reason()};

                   (ssh:connection_ref(),
                    [sftp_option()]
                   )
                   -> {ok,pid()}  | {ok,pid(),ssh:connection_ref()} | {error,reason()};

                   (ssh:host(),
                    [ssh:client_option() | sftp_option()]
                   )
                   -> {ok,pid(),ssh:connection_ref()} | {error,reason()} .
start_channel(Cm, UserOptions0) when is_pid(Cm) ->
    UserOptions = legacy_timeout(UserOptions0),
    Timeout = proplists:get_value(timeout, UserOptions, infinity),
    {_SshOpts, ChanOpts, SftpOpts} = handle_options(UserOptions),
    WindowSize = proplists:get_value(window_size, ChanOpts, ?XFER_WINDOW_SIZE),
    PacketSize = proplists:get_value(packet_size, ChanOpts, ?XFER_PACKET_SIZE),
    case ssh_connection:session_channel(Cm, WindowSize, PacketSize, Timeout) of
	{ok, ChannelId} ->
            case ssh_connection_handler:start_channel(Cm, ?MODULE, ChannelId,
                                                      [Cm,ChannelId,SftpOpts], undefined) of
		{ok, Pid} ->
		    case wait_for_version_negotiation(Pid, Timeout) of
			ok ->
			    {ok, Pid};
			TimeOut ->
			    TimeOut
		    end;
		{error, Reason} ->
		    {error, format_channel_start_error(Reason)}
	    end;
	Error ->
	    Error
    end;

I could not determine exactly what is the type of TimeOut and I could not figure out if that can actually happen, but could it be that TimeOut doesn't comply with the spec? It's hard to see the connection with connect_timeout so maybe it's not interesting but I thought of mentioning it.

Affected versions
Erlang/OTP 27.2 and master (tested with commit 53ffb6a)

Additional context
If instead of passing connect_timeout, timeout is used, then no warning is detected by dialyzer. So if the following code is analysed instead no warning occurs:

f() ->
    {ok, Pid} = ssh:connect("127.0.0.1", 22, []),
    ssh_sftp:start_channel(Pid, [{timeout, 1000}]),
   ok.

and also

f() ->
    {ok, Pid} = ssh:connect("127.0.0.1", 22, []),
    ssh_sftp:start_channel(Pid, [{connect_timeout, 1000}, {timeout, 1000}]),
   ok.
@alexandrejbr alexandrejbr added the bug Issue is reported as a bug label Jan 29, 2025
@IngelaAndin IngelaAndin added the team:PS Assigned to OTP team PS label Jan 30, 2025
@jhogberg jhogberg self-assigned this Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug team:PS Assigned to OTP team PS
Projects
None yet
Development

No branches or pull requests

3 participants