search

php / pecl-networking-ssh2

Bindings for the libssh2 library
http://pecl.php.net/package/ssh2
Other
51 stars 58 forks source link

Expose libssh2_session_set_timeout() #70

Open Jille opened 1 year ago

Jille commented 1 year ago

issue #62

Jille commented 1 year ago

Not tested. I was hoping travis-ci would help me with that, but it doesn't seem to do much: https://app.travis-ci.com/github/php/pecl-networking-ssh2/pull_requests

langemeijer commented 1 year ago

Thank you for your time and your contribution. I have three points I'd like to discuss with you:

Point 1: Triggered by your PR I started looking for the connect timeout (Which obviously you cannot set using your new ssh2_session_set_timeout() method because there is no session yet.) I found that as connect timeout value ssh2_connect() uses default-socket-timeout

I have always thought that this would be the default read/write timeout too, but apparently it is not. I think this is a bug. I think we have to add a libssh2_session_set_timeout() call to ssh2_connect(). Because the default value in PHP is 60 seconds, it's unlikely that this BC break would cause any problems for our users.

Point 2: I'd like the function to be named ssh2_set_timeout(). The whole concept of libssh2 session and channels is not as such implemented in the ssh2 extension. For our users a session is just the name of the connection resource, most methods work on session.

Point 3: I'd prefer that the function would follow the same parameters as stream_set_timeout: ssh2_set_timeout(resource $session, int $seconds, int $microseconds = 0)

Although we have a problem here. set_stream_timeout() allows for microseconds, but libssh2_session_set_timeout() 'only' has a millisecond resolution. We would have to round $microseconds up to a multiple of 1000 and then divide by 1000.

I like the consistency between methods and also: Using sub-second precision is probably and edge-case. With my suggested call signature the seconds-case is made easy, still allowing for more precision when needed.

What do you think?

langemeijer commented 1 year ago

I have changed my mind on point 1. ssh2_exec($connection, "sleep 100;") would timeout at 60 seconds. Any PHP script running a task remotely for more than 60 without any output would change behaviour. I think that this is quite a normal use-case, think of dumping a database to file. I don't think having a default timeout is a good idea.

I tried messing about with libssh2_keepalive_config() but that didn't lead to anything useful.

Jille commented 1 year ago

Thanks for the elaborate response Casper. My apologies for the slow reply. Time is hard to find these days.

  1. Agreed we should not make a change that'd give most people a 60s timeout. That'd break a lot of people unexpectedly.

2+3. Okay!

Groetjes,

mrngm commented 1 year ago

(apologies for the noise, it seems "Sync fork" on Github leaves a (somewhat unnecessary) merge commit)

Jille commented 1 month ago

Anything I can do to move this forward?