Skip to content

Commit 23c24b7

Browse files
TimandesTimandes
committed
fix: optimize callback-related statements
1 parent 70ae51d commit 23c24b7

File tree

6 files changed

+48
-43
lines changed

6 files changed

+48
-43
lines changed

‎php_zookeeper.c‎

Lines changed: 15 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -314,7 +314,7 @@ static PHP_METHOD(Zookeeper, getChildren)
314314
(fci.size != 0) ? php_zk_node_watcher_marshal : NULL,
315315
cb_data, &strings);
316316
if (status != ZOK) {
317-
php_cb_data_destroy(&cb_data);
317+
php_cb_data_remove(cb_data);
318318
php_zk_throw_exception(status TSRMLS_CC);
319319
return;
320320
}
@@ -363,40 +363,30 @@ static PHP_METHOD(Zookeeper, get)
363363
status = zoo_exists(i_obj->zk, path, 0, &stat);/* I think we don't need zk->watcher any more */
364364

365365
if (status != ZOK) {
366-
php_cb_data_destroy(&cb_data);
366+
php_cb_data_remove(cb_data);
367367
php_zk_throw_exception(status TSRMLS_CC);
368368
return;
369369
}
370-
length = stat.dataLength;
370+
371+
length = (stat.dataLength > 0)?stat.dataLength:1; // At least one byte
371372
} else {
372373
length = max_size;
373374
}
374375

375-
if (length <= 0) {/* znode carries a NULL */
376-
if (stat_info) {
377-
php_stat_to_array(&stat, stat_info);
378-
}
379-
380-
php_cb_data_destroy(&cb_data);
381-
RETURN_NULL();
382-
}
376+
/* We should not break the procedure here
377+
because if znode carries a NULL,
378+
cb_data will lose it's use */
383379

384380
php_zk_log_info(i_obj->zk, "path=%s, cb_data=%p", path, cb_data);
385381

386-
buffer = emalloc (length+1);
382+
buffer = emalloc (length);
387383
status = zoo_wget(i_obj->zk, path, (fci.size != 0) ? php_zk_node_watcher_marshal : NULL,
388384
cb_data, buffer, &length, &stat);
389-
buffer[length] = 0;
390385

391386
if (status != ZOK) {
392387
efree (buffer);
393-
php_cb_data_destroy(&cb_data);
388+
php_cb_data_remove(cb_data);
394389
php_zk_throw_exception(status TSRMLS_CC);
395-
396-
/* Indicate data marshalling failure with boolean false so that user can retry */
397-
if (status == ZMARSHALLINGERROR) {
398-
RETURN_FALSE;
399-
}
400390
return;
401391
}
402392

@@ -406,7 +396,6 @@ static PHP_METHOD(Zookeeper, get)
406396

407397
/* Length will be returned as -1 if the znode carries a NULL */
408398
if (length == -1) {
409-
php_cb_data_destroy(&cb_data);
410399
RETURN_NULL();
411400
}
412401

@@ -441,7 +430,7 @@ static PHP_METHOD(Zookeeper, exists)
441430
status = zoo_wexists(i_obj->zk, path, (fci.size != 0) ? php_zk_node_watcher_marshal : NULL,
442431
cb_data, &stat);
443432
if (status != ZOK && status != ZNONODE) {
444-
php_cb_data_destroy(&cb_data);
433+
php_cb_data_remove(cb_data);
445434
php_zk_throw_exception(status TSRMLS_CC);
446435
return;
447436
}
@@ -712,7 +701,7 @@ static PHP_METHOD(Zookeeper, setWatcher)
712701
ZK_METHOD_FETCH_OBJECT;
713702

714703
if (i_obj->cb_data) {
715-
zend_hash_index_del(&i_obj->callbacks, i_obj->cb_data->h);
704+
php_cb_data_remove(i_obj->cb_data);
716705
}
717706
cb_data = php_cb_data_new(&i_obj->callbacks, &fci, &fcc, 0 TSRMLS_CC);
718707
zoo_set_watcher(i_obj->zk, php_zk_watcher_marshal);
@@ -820,7 +809,7 @@ static void php_zk_close(php_zk_t *i_obj TSRMLS_DC)
820809
// stored in the Zookeeper instance. The destructor of the callbacks hashtable
821810
// (php_cb_data_zv_destroy) already frees the callback data. Below line resulted
822811
// in a double free, which triggers segfaults.
823-
//php_cb_data_destroy(&i_obj->cb_data);
812+
php_cb_data_remove(i_obj->cb_data);
824813
i_obj->cb_data = NULL;
825814
}
826815

@@ -850,8 +839,7 @@ static void php_zk_free_storage(zend_object *obj TSRMLS_DC)
850839
static void php_cb_data_zv_destroy(zval *entry)
851840
{
852841
if( Z_TYPE_P(entry) == IS_PTR ) {
853-
php_cb_data_destroy(Z_PTR_P(entry)); // php_cb_data_t
854-
efree(Z_PTR_P(entry)); // Allocated by zend_hash_next_index_insert_mem()
842+
php_cb_data_destroy((php_cb_data_t *)Z_PTR_P(entry)); // php_cb_data_t
855843
}
856844
}
857845

@@ -892,7 +880,7 @@ static inline void php_zk_dispatch_one(php_cb_data_t *cb_data, int type, int sta
892880
zval_ptr_dtor(&params[2]);
893881

894882
if (cb_data->oneshot) {
895-
zend_hash_index_del(cb_data->ht, cb_data->h);
883+
php_cb_data_remove(cb_data);
896884
}
897885
}
898886

@@ -915,7 +903,7 @@ static inline void php_zk_dispatch_one_completion(php_cb_data_t *cb_data, int rc
915903
}
916904

917905
if (cb_data->oneshot) {
918-
zend_hash_index_del(cb_data->ht, cb_data->h);
906+
php_cb_data_remove(cb_data);
919907
}
920908
}
921909

‎php_zookeeper_callback.c‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,19 +27,26 @@ php_cb_data_t* php_cb_data_new(HashTable *ht, zend_fcall_info *fci, zend_fcall_i
2727
cbd->oneshot = oneshot;
2828
cbd->h = ht->nNextFreeElement;
2929
Z_TRY_ADDREF(cbd->fci.function_name);
30-
zend_hash_next_index_insert_mem(ht, (void*)&cbd, sizeof(php_cb_data_t *));
30+
zend_hash_next_index_insert_ptr(ht, (void*)cbd);
3131
cbd->ht = ht;
3232
#if ZTS
3333
TSRMLS_SET_CTX(cbd->ctx);
3434
#endif
3535
return cbd;
3636
}
3737

38-
void php_cb_data_destroy(php_cb_data_t **entry)
38+
void php_cb_data_destroy(php_cb_data_t *cbd)
3939
{
40-
php_cb_data_t *cbd = *(php_cb_data_t **)entry;
4140
if (cbd) {
4241
Z_TRY_DELREF(cbd->fci.function_name);
4342
efree(cbd);
4443
}
4544
}
45+
46+
void php_cb_data_remove(php_cb_data_t *cb_data)
47+
{
48+
if (cb_data && cb_data->ht) {
49+
zend_hash_index_del(cb_data->ht, cb_data->h);
50+
}
51+
php_cb_data_destroy(cb_data);
52+
}

‎php_zookeeper_callback.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ typedef struct _php_cb_data_t {
3232
} php_cb_data_t;
3333

3434
php_cb_data_t* php_cb_data_new(HashTable *ht, zend_fcall_info *fci, zend_fcall_info_cache *fcc, zend_bool oneshot TSRMLS_DC);
35-
void php_cb_data_destroy(php_cb_data_t **entry);
35+
void php_cb_data_destroy(php_cb_data_t *cbd);
36+
void php_cb_data_remove(php_cb_data_t *cb_data);
3637

37-
#endif /* PHP_ZOOKEEPER_CALLBACK */
38+
#endif /* PHP_ZOOKEEPER_CALLBACK */

‎php_zookeeper_config_class.c‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -83,20 +83,14 @@ static PHP_METHOD(ZookeeperConfig, get)
8383
cb_data = php_cb_data_new(&i_obj->php_zk->callbacks, &fci, &fcc, 1 TSRMLS_CC);
8484
}
8585

86-
buffer = emalloc (length+1);
86+
buffer = emalloc(length);
8787
status = zoo_wgetconfig(i_obj->php_zk->zk, (fci.size != 0) ? php_zk_watcher_marshal : NULL,
8888
cb_data, buffer, &length, &stat);
89-
buffer[length] = 0;
9089

9190
if (status != ZOK) {
9291
efree (buffer);
93-
php_cb_data_destroy(&cb_data);
92+
php_cb_data_remove(cb_data);
9493
php_zk_throw_exception(status TSRMLS_CC);
95-
96-
/* Indicate data marshalling failure with boolean false so that user can retry */
97-
if (status == ZMARSHALLINGERROR) {
98-
RETURN_FALSE;
99-
}
10094
return;
10195
}
10296

‎tests/retrieve_node_with_watcher_callback.phpt‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,28 @@ if (!extension_loaded('zookeeper'))
99
<?php
1010
class Test extends Zookeeper
1111
{
12+
private $path = '';
13+
1214
public function watcher($i, $type, $key)
1315
{
14-
$this->get('/zookeeper', array($this, 'watcher'));
16+
$this->get($this->path, array($this, 'watcher'));
17+
}
18+
19+
public function setPath(string $path): void
20+
{
21+
$this->path = $path;
1522
}
1623
}
1724

1825
$test = new Test('127.0.0.1:2181');
19-
echo gettype($test->get('/zookeeper', array($test, 'watcher')));
26+
27+
$path = '/test-node-' . uniqid();
28+
$test->create($path, null);
29+
30+
$test->setPath($path);
31+
echo gettype($test->get($path, array($test, 'watcher')));
32+
33+
$test->delete($path);
34+
2035
--EXPECT--
2136
NULL

‎tests/retrieve_null_when_get_empty_node.phpt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ if (!extension_loaded('zookeeper'))
88
--FILE--
99
<?php
1010
$client = new Zookeeper('localhost:2181');
11-
$client->create('/test1', '', array(
11+
$client->create('/test1', null, array(
1212
array(
1313
'perms' => Zookeeper::PERM_ALL,
1414
'scheme' => 'world',

0 commit comments

Comments
 (0)