UserOnlineTable::setOnline() - Code Metrics - Inspection of "Fixes race condition on marking user online" - Schlaefer/Saito - Measure and Improve Code Quality continuously with Scrutinizer

Completed

Push — develop ( 207923...b9ffce )

by Schlaefer

created 2019-08-08 19:25 UTC

UserOnlineTable::setOnline() B

↳ Parent: UserOnlineTable

Complexity

Conditions	7
Paths	8

Size

Total Lines

Duplication

Lines	0
Ratio	0 %

Importance

Changes

Metric	Value
cc	7
nc	8
nop	2
dl	0
loc	54
rs	8.0703
c	0
b	0
f	0

How to fix Long Method

<?php

declare(strict_types=1);

/**
 * Saito - The Threaded Web Forum
 *
 * @copyright Copyright (c) the Saito Project Developers
 * @link https://github.com/Schlaefer/Saito
 * @license http://opensource.org/licenses/MIT
 */

namespace App\Model\Table;

use Cake\Log\LogTrait;
use Cake\ORM\Query;
use Cake\ORM\Table;
use Cake\Validation\Validator;
use Psr\Log\LogLevel;
use Stopwatch\Lib\Stopwatch;

/**
 * Stores which users are online
 *
 * Storage can be nopersistent as it is constantly rebuild with live-data.
 *
 * Field notes:
 * - `time` - Timestamp as int unix-epoch instead regular DATETIME. Makes it
 *   cheap to clear out-timed users by comparing int values.
 */
class UserOnlineTable extends Table
{
    use LogTrait;

    /**
     * Time in seconds until a user is considered offline
     *
     * @var int
     */
    public $timeUntilOffline = 1200;

    /**
     * {@inheritDoc}
     */
    public function initialize(array $config)
    {
        $this->setTable('useronline');

        $this->addBehavior('Timestamp');

        $this->addBehavior(
            'Cron.Cron',
            [
                'gc' => [
                    'id' => 'UserOnline.deleteGone',
                    'due' => '+1 minutes',
                ],
            ]
        );

        $this->belongsTo(
            'Users',
            [
                'foreignKey' => 'user_id'
            ]
        );
    }

    /**
     * {@inheritDoc}
     */
    public function validationDefault(Validator $validator)
    {
        $validator

            //= uuid
            ->notEmpty('uuid')
            ->requirePresence('uuid')
            ->add(
                'uuid',
                [
                    'isUnique' => [
                        'rule' => 'validateUnique',
                        'provider' => 'table'
                    ],
                ]
            );

        return $validator;
    }

    /**
     * Sets user with `$id` online
     *
     * @param string $id usually user-ID (logged-in user) or session_id (not logged-in)
     * @param bool $loggedIn user is logged-in
     * @return void
     */
    public function setOnline(string $id, bool $loggedIn): void
    {
        $now = time();
        $id = $this->getShortendedId((string)$id);

        $user = $this->find()->where(['uuid' => $id])->first();

        if ($user) {
            // [Performance] Only hit database if timestamp is about to get outdated.
            //
            // Adjust to sane values taking JS-frontend status ping time
            // intervall into account.
            if ($user->get('time') < ($now - (int)($this->timeUntilOffline * 80 / 100))) {
                $user->set('time', $now);
                $this->save($user);
/**
 * @return array|string
 */
function returnsDifferentValues($x) {
    if ($x) {
        return 'foo';
    }

    return array();
}

$x = returnsDifferentValues($y);
if (is_array($x)) {
    // $x is an array.
}
            }

            return;
        }

        $data = ['logged_in' => $loggedIn, 'time' => $now, 'uuid' => $id];
        if ($loggedIn) {
            $data['user_id'] = (int)$id;
        }
        $user = $this->newEntity($data);

        try {
            $this->save($user);
        } catch (\PDOException $e) {
            // We saw that some mobile browsers occasionaly send two requests at
            // the same time. On of the two requests was always the status-ping.
            // Working theory: cause is a power-coalesced status-ping now
            // bundled with a page reload on tab-"resume" esp. with http/2.
            //
            // When the second request arrives (ns later) the first request
            // hasn't persistet its save to the DB yet (which happens in the ms
            // range). So the second request doesn't see the user online and
            // tries to save the same "uuid" again. The DB will not have any of
            // that nonsense after it set the "uuid" from the first request on a
            // unique column, and raises an error which may be experienced by
            // the user.
            //
            // Since the first request did the necessary work of marking the
            // user online, we suppress this error, assuming it will only happen
            // in this particular situation. *knocks on wood*
            if ($e->getCode() == 23000 && strstr($e->getMessage(), 'uuid')) {
                $this->log(
                    'Cought duplicate "uuid" key exception in UserOnline::setOnline.',
                    LogLevel::INFO,
                    'saito.info'
                );
            }
        }
    }

    /**
     * Removes user with uuid `$id` from UserOnline
     *
     * @param int|string $id id
     * @return void
     */
    public function setOffline($id): void
    {
        $id = $this->getShortendedId((string)$id);
        $this->deleteAll(['UserOnline.uuid' => $id]);
    }

    /**
     * Get all logged-in users
     *
     * Don't use directly but use \Saito\App\Stats
     *
     * @td @sm make finder
     *
     * @return Query
     */
    public function getLoggedIn(): Query
    {
        Stopwatch::start('UserOnline->getLoggedIn()');
        $loggedInUsers = $this->find(
            'all',
            [
                'contain' => [
                    'Users' => [
                        'fields' => ['id', 'user_type', 'username']
                    ]
                ],
                'conditions' => ['UserOnline.logged_in' => true],
                'fields' => ['id'],
                'order' => ['LOWER(Users.username)' => 'ASC']
            ]
        );
        Stopwatch::stop('UserOnline->getLoggedIn()');

        return $loggedInUsers;
    }

    /**
     * Removes users which weren't online $timeDiff seconds
     *
     * @return void
     */
    public function gc(): void
    {
        $this->deleteAll(['time <' => time() - ($this->timeUntilOffline)]);
    }

    /**
     * Shortens a string to fit in the uuid table-field
     *
     * @param string $id string
     * @return string
     */
    protected function getShortendedId(string $id)
    {
        return substr($id, 0, 32);
    }
}


1			<?php
2
3			declare(strict_types=1);
4
5			/**
6			* Saito - The Threaded Web Forum
7			*
8			* @copyright Copyright (c) the Saito Project Developers
9			* @link https://github.com/Schlaefer/Saito
10			* @license http://opensource.org/licenses/MIT
11			*/
12
13			namespace App\Model\Table;
14
15			use Cake\Log\LogTrait;
16			use Cake\ORM\Query;
17			use Cake\ORM\Table;
18			use Cake\Validation\Validator;
19			use Psr\Log\LogLevel;
20			use Stopwatch\Lib\Stopwatch;
21
22			/**
23			* Stores which users are online
24			*
25			* Storage can be nopersistent as it is constantly rebuild with live-data.
26			*
27			* Field notes:
28			* - `time` - Timestamp as int unix-epoch instead regular DATETIME. Makes it
29			* cheap to clear out-timed users by comparing int values.
30			*/
31			class UserOnlineTable extends Table
32			{
33			use LogTrait;
34
35			/**
36			* Time in seconds until a user is considered offline
37			*
38			* @var int
39			*/
40			public $timeUntilOffline = 1200;
41
42			/**
43			* {@inheritDoc}
44			*/
45			public function initialize(array $config)
46			{
47			$this->setTable('useronline');
48
49			$this->addBehavior('Timestamp');
50
51			$this->addBehavior(
52			'Cron.Cron',
53			[
54			'gc' => [
55			'id' => 'UserOnline.deleteGone',
56			'due' => '+1 minutes',
57			],
58			]
59			);
60
61			$this->belongsTo(
62			'Users',
63			[
64			'foreignKey' => 'user_id'
65			]
66			);
67			}
68
69			/**
70			* {@inheritDoc}
71			*/
72			public function validationDefault(Validator $validator)
73			{
74			$validator
			0 ignored issues – show Deprecated Code introduced 2019-07-31 15:52 UTC by Report Bug Copy Issue Report The method `Cake\Validation\Validator::notEmpty()` has been deprecated with message: 3.7.0 Use notEmptyString(), notEmptyArray(), notEmptyFile(), notEmptyDate(), notEmptyTime() or notEmptyDateTime() instead. This method has been deprecated. The supplier of the class has supplied an explanatory message. The explanatory message should give you some clue as to whether and when the method will be removed from the class and what other method or class to use instead. Loading history...
75			//= uuid
76			->notEmpty('uuid')
77			->requirePresence('uuid')
78			->add(
79			'uuid',
80			[
81			'isUnique' => [
82			'rule' => 'validateUnique',
83			'provider' => 'table'
84			],
85			]
86			);
87
88			return $validator;
89			}
90
91			/**
92			* Sets user with `$id` online
93			*
94			* @param string $id usually user-ID (logged-in user) or session_id (not logged-in)
95			* @param bool $loggedIn user is logged-in
96			* @return void
97			*/
98			public function setOnline(string $id, bool $loggedIn): void
99			{
100			$now = time();
101			$id = $this->getShortendedId((string)$id);
102
103			$user = $this->find()->where(['uuid' => $id])->first();
104
105			if ($user) {
106			// [Performance] Only hit database if timestamp is about to get outdated.
107			//
108			// Adjust to sane values taking JS-frontend status ping time
109			// intervall into account.
110			if ($user->get('time') < ($now - (int)($this->timeUntilOffline * 80 / 100))) {
111			$user->set('time', $now);
112			$this->save($user);
			0 ignored issues – show Bug introduced 2019-05-16 13:57 UTC by Report Bug Copy Issue Report It seems like `$user` defined by `$this->find()->where(arr...uuid' => $id))->first()` on line `103` can also be of type `array`; however, `Cake\ORM\Table::save()` does only seem to accept `object<Cake\Datasource\EntityInterface>`, maybe add an additional type check? If a method or function can return multiple different values and unless you are sure that you only can receive a single value in this context, we recommend to add an additional type check: /** * @return array\|string */ function returnsDifferentValues($x) { if ($x) { return 'foo'; } return array(); } $x = returnsDifferentValues($y); if (is_array($x)) { // $x is an array. } If this a common case that PHP Analyzer should handle natively, please let us know by opening an issue. Loading history...
113			}
114
115			return;
116			}
117
118			$data = ['logged_in' => $loggedIn, 'time' => $now, 'uuid' => $id];
119			if ($loggedIn) {
120			$data['user_id'] = (int)$id;
121			}
122			$user = $this->newEntity($data);
123
124			try {
125			$this->save($user);
126			} catch (\PDOException $e) {
127			// We saw that some mobile browsers occasionaly send two requests at
128			// the same time. On of the two requests was always the status-ping.
129			// Working theory: cause is a power-coalesced status-ping now
130			// bundled with a page reload on tab-"resume" esp. with http/2.
131			//
132			// When the second request arrives (ns later) the first request
133			// hasn't persistet its save to the DB yet (which happens in the ms
134			// range). So the second request doesn't see the user online and
135			// tries to save the same "uuid" again. The DB will not have any of
136			// that nonsense after it set the "uuid" from the first request on a
137			// unique column, and raises an error which may be experienced by
138			// the user.
139			//
140			// Since the first request did the necessary work of marking the
141			// user online, we suppress this error, assuming it will only happen
142			// in this particular situation. knocks on wood
143			if ($e->getCode() == 23000 && strstr($e->getMessage(), 'uuid')) {
144			$this->log(
145			'Cought duplicate "uuid" key exception in UserOnline::setOnline.',
146			LogLevel::INFO,
147			'saito.info'
148			);
149			}
150			}
151			}
152
153			/**
154			* Removes user with uuid `$id` from UserOnline
155			*
156			* @param int\|string $id id
157			* @return void
158			*/
159			public function setOffline($id): void
160			{
161			$id = $this->getShortendedId((string)$id);
162			$this->deleteAll(['UserOnline.uuid' => $id]);
163			}
164
165			/**
166			* Get all logged-in users
167			*
168			* Don't use directly but use \Saito\App\Stats
169			*
170			* @td @sm make finder
171			*
172			* @return Query
173			*/
174			public function getLoggedIn(): Query
175			{
176			Stopwatch::start('UserOnline->getLoggedIn()');
177			$loggedInUsers = $this->find(
178			'all',
179			[
180			'contain' => [
181			'Users' => [
182			'fields' => ['id', 'user_type', 'username']
183			]
184			],
185			'conditions' => ['UserOnline.logged_in' => true],
186			'fields' => ['id'],
187			'order' => ['LOWER(Users.username)' => 'ASC']
188			]
189			);
190			Stopwatch::stop('UserOnline->getLoggedIn()');
191
192			return $loggedInUsers;
193			}
194
195			/**
196			* Removes users which weren't online $timeDiff seconds
197			*
198			* @return void
199			*/
200			public function gc(): void
201			{
202			$this->deleteAll(['time <' => time() - ($this->timeUntilOffline)]);
203			}
204
205			/**
206			* Shortens a string to fit in the uuid table-field
207			*
208			* @param string $id string
209			* @return string
210			*/
211			protected function getShortendedId(string $id)
212			{
213			return substr($id, 0, 32);
214			}
215			}
216

Schlaefer / Saito

Push — develop ( 207923...b9ffce )

UserOnlineTable::setOnline() B

Complexity

Size

Duplication

Importance

How to fix Long Method

Long Method

Duplication Side-by-Side

Filter issues like