fix order of user_error() and bitmap reset

[sebsel]

In some configurations of PHP (at least in mine), execution stops after a user_error(), so the bitmap is never set to 0 in that case. I think it's best just always first set the bitmap and then do the error.

[bantu]

Hmm. If execution stops, the value of bitmap is irrelevant. Can you elaborate?

[bantu]

0c26415 went into 1.0, so this should too.

[bantu]

Feel free to rebase onto 1.0. Patch applies cleanly.

[sebsel]

I'm a bit confused by how the branches relate. First off: I can't rebase onto 1.0 cleanly. It gives me a lot of conflicts to resolve. It would be faster if I got a new branch from 1.0 and manually fix the thing. I can do that.

But next: on 2.0 there are, as you can see in this PR, 4 places where there is this order of error and reset, but on 1.0 there is just one. Fixing this for 1.0 won't fix those cases in 2.0, and the package I'm using in my project is using 2.0 (per it's composer.json).

The problem I try to solve is described in short in #1298 and more in dept in thephpleague/flysystem-sftp#66.

It's not really that the execution stops, but it throws, which means the next line won't run. If the throw is caught, $bitmap remains 15 (in my case) so isConnected() will stay true, meaning flysystem-sftp will not try to reconnect.

I think there is no harm in first doing the $this->bitmap = 0 and then the user_error(). I'm not adding, only rearranging. But I do think it has to be like this for all the 4 places in 2.0 and the 1 remaining place in 1.0.

I can give you steps to reproduce if you want.

[sebsel]

Steps to reproduce:

1. create a fresh install of the Laravel framework (with the Laravel installer this is laravel new fresh, for a project called 'fresh')

2. do a composer require league/flysystem-sftp to enable Sftp support (just found out that this is all you need in the new Laravel 5.7, yaay)

3. open config/filesystems.php and add this item in the array under 'disks' => [:

   'sftp' => [
       'driver' => 'sftp',
       'root' => '/home/user/',
       'host' => 'example.com',
       'username' => 'user',
       'password' => 'password',
   ],
   

   ... with the right configuration for your server.

4. Do a php artisan tinker to open the build-in REPL. Then call Storage::disk('sftp')->files();. It will list your files.

5. SSH into the machine yourself, do a ps faux. There will be two SSH-sessions, one running ps faux. Kill the other with sudo kill 123 where 123 is the pid.

6. Call Storage::disk('sftp')->files(); again: it will fail with 'Connection closed prematurely'. Call Storage::disk('sftp')->getAdapter()->isConnected();: it will be true. Call ->files() again: it will fail again.

Now, exit tinker, apply patch from PR in the vendor folder, and start again at 4. Step 6 will now look like:

• Call Storage::disk('sftp')->files(); again: it will fail with 'Connection closed prematurely'. Call Storage::disk('sftp')->getAdapter()->isConnected();: it will be false. Call ->files() again: it will list your files again.

Which means: in a long-running queue worker when connection has dropped due to a night of no-activity, failed jobs that are retried will start working again. Also: the first job of the day fails, but next jobs will have connection again.

[bantu]

@sebsel Thanks a lot for your detailed explanation.

I'm a bit confused by how the branches relate. First off: I can't rebase onto 1.0 cleanly. It gives me a lot of conflicts to resolve. It would be faster if I got a new branch from 1.0 and manually fix the thing. I can do that.

The idea is to forward merge 1.0 into 2.0 into master.

But next: on 2.0 there are, as you can see in this PR, 4 places where there is this order of error and reset, but on 1.0 there is just one. Fixing this for 1.0 won't fix those cases in 2.0, and the package I'm using in my project is using 2.0 (per it's composer.json).

You're absolutely right. I was confused because cherry-picking the commit onto 1.0 does not result in conflicts. I've submitted PR #1314 for 1.0.

The problem I try to solve is described in short in #1298 and more in dept in thephpleague/flysystem-sftp#66.

I think there is no harm in first doing the $this->bitmap = 0 and then the user_error(). I'm not adding, only rearranging. But I do think it has to be like this for all the 4 places in 2.0 and the 1 remaining place in 1.0.

I am fine with this. However ...

It's not really that the execution stops, but it throws, which means the next line won't run. If the throw is caught, $bitmap remains 15 (in my case) so isConnected() will stay true, meaning flysystem-sftp will not try to reconnect.

I think, if the behaviour of user_error() is not reliable, we should not use it in the first place.

[bantu]

I think, if the behaviour of user_error() is not reliable, we should not use it in the first place.

Great, master already uses exceptions.

[terrafrost]

I think, if the behaviour of user_error() is not reliable, we should not use it in the first place.

The issue is presumably due to the use of a custom error handler.

Consider the following code:

<?php

function error_handler($errno, $errstr, $errfile, $errline)
{
    exit("all done!\n");
}

set_error_handler('error_handler');

trigger_error('sample error', E_USER_NOTICE);

echo "this far\n";

The "this far" never gets output whereas the "all done!" does.