[NFR] Pull request for interface checking #745
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I'm adding a check in PHALCON_THROW_EXCEPTION to check if there is an active memory frame, is better instead adding a new macro |
|
OK, this seems to be a much better solution as it eliminates the possibility of a human error. |
|
Also, thanks for the work, but we don't want to check for interfaces, there are several methods that accept objects and checking in every request the correct interface would be a lot of overhead. We were thinking in implement a dev version or some compilation flag for that. |
|
A benchmark ad deliberandum: https://gist.github.com/sjinks/5894540
Every function is run 10,000,000 times. My results:
Type checking by Zend Engine is the slowest; Thus it probably makes sense to enforce the correct object type for rarely called functions like setDI() or setEventsManager() — this will ensure that the passed objects implement the expected interface and will help to diagnose some errors much earlier. |
|
After the last changes my app crashes. I noticed that the apache logs shows me: In index.php on line 21 I have: echo $app->handle()->getContent();What could be wrong? |
|
|
@sjinks we can add this change in 1.3.0, 1.2.0 will be released next weekend, so we don't plan to add new features or break something, thank you |
|
Yeah, sure |
|
@sjinks |
|
Just run the base-app (branch 1.2). |
|
You don't need -dbg packages to generate a core. ulimit -c unlimited
# Note that directories/ files in /tmp may not survive across reboots
mkdir -p /tmp/apache2-gdb-dump
chmod 0777 /tmp/apache2-gdb-dumpThen add to your apache config and restart apache. When PHP crashes, you will see a file (typically named Then please run To make the backtrace more useful, you may want to recompile phalcon like this: phpize
export CFLAGS="-O0 -g3"
./configure --enable-phalcon
make
sudo make install
service httpd restart
# or `service apache2 restart`, I don't know how it is called in SuSE |
|
I've done this before but to no avail. ulimit to /etc/sysconfig/apache2 create folder CoreDump option in /etc/apache2/httpd.conf After restart apache and php crashes, the dir is still empty, so I thought that these packages are required. |
|
OK, then if this is not a production server, please stop apache, then run Then try to open a page where the crash happens; when the app crashes, you will see a not in gdb; please use |
|
When I run And So, |
|
Try getting the backtrace from console: $ cd /srv/www/htdocs/your-dir/public
$ gdb php
gdb> run index.php
gdb> bt |
On line 148 in Bootstrap I have the session service. Strange because, previously worked well. |
|
It seems that the user which you're trying to get the backtrace does not have write permissions in the directory: /var/lib/php5/, can you try get the backtrace with root? |
|
Please try |
|
@mruz Please apply this patch to your base-app and see if the error is gone. |
|
OK, I was able to generate a backtrace: bt full: |
|
And another one: tl;dr version: |
|
What PHP code is producing the backtrace? |
|
This: https://github.com/mruz/base-app/tree/1.2 For me the app crashes only on /user/signup (not always); as far as I can tell, the app crashes only when there's a Referer: field in the headers (I was unable to crash it using direct requests). Crashes only under Apache. |
|
This PHP code always crashes under cli: <?php
/**
* index.php
*
* @package base-app
* @category Running
* @version 1.2
*/
error_reporting(E_ALL);
$_SERVER['HTTP_REFERER'] = 'http://www.google.com/';
$_GET['_url'] = '/user/signup';
try {
if (!defined('ROOT_PATH')) {
define('ROOT_PATH', dirname(dirname(__FILE__)));
}
require_once ROOT_PATH . '/app/Bootstrap.php';
$app = new \Bootstrap(new \Phalcon\DI\FactoryDefault());
echo $app->handle()->getContent();
} catch (\Phalcon\Exception $e) {
Bootstrap::log($e);
} catch (\PDOException $e) {
Bootstrap::log($e);
} catch (\Exception $e) {
Bootstrap::log($e);
}If you comment out |
|
One important thing: I did NOT configure the database. Pretty interesting: with If I remove ANY of the |
|
I think this can be related to restore the memory frame without MM_GROW when using PHALCON_THROW_EXCEPTION_STR, I think we finally would need to introduce PHALCON_THROW_EXCEPTION_STRW |
|
@phalcon Here is the code that reproduces the crash from CLI: https://github.com/sjinks/crash-phalcon Looks like the issue lies somewhere in Volt. Any of the following stops crashes:
https://github.com/sjinks/crash-phalcon/commit/843bad2d51b7c82eb440055e3b2d0d9a1b5d874e |
|
My test code does not seem to throw any exceptions… |
|
I think we will be able to trace down the bug: the app does not crash when used with 6e4698d |
|
I'm also using the DLL for windows compiled some days ago and the bug is not in there, I think we introduced it recently |
|
No, these are not exceptions… git checkout 5116359
git checkout 7d58ea3 ext/mvc/view/engine/volt/compiler.c
cd ext
phpize --clean && phpize && ./configure && make -B -j15 && sudo make installWith this configuration the app crashes. These are the changes in the code: 7d58ea3#diff-15 BTW, when is it right to use PHALCON_OBS_VAR()? |
|
These changes fix the crash: diff --git a/ext/mvc/view/engine/volt/compiler.c b/ext/mvc/view/engine/volt/compiler.c
index 21c841a..74b90b3 100644
--- a/ext/mvc/view/engine/volt/compiler.c
+++ b/ext/mvc/view/engine/volt/compiler.c
@@ -3591,7 +3591,7 @@ PHP_METHOD(Phalcon_Mvc_View_Engine_Volt_Compiler, compile){
* This makes that templates will be compiled always
*/
if (phalcon_array_isset_string(options, SS("compileAlways"))) {
-
+ PHALCON_OBS_NVAR(compile_always);
phalcon_array_fetch_string(&compile_always, options, SL("compileAlways"), PH_NOISY_CC);
if (Z_TYPE_P(compile_always) != IS_BOOL) {
PHALCON_THROW_EXCEPTION_STR(phalcon_mvc_view_exception_ce, "compileAlways must be a bool value");
@@ -3603,7 +3603,7 @@ PHP_METHOD(Phalcon_Mvc_View_Engine_Volt_Compiler, compile){
* Prefix is prepended to the template name
*/
if (phalcon_array_isset_string(options, SS("prefix"))) {
-
+ PHALCON_OBS_NVAR(prefix);
phalcon_array_fetch_string(&prefix, options, SL("prefix"), PH_NOISY_CC);
if (Z_TYPE_P(prefix) != IS_STRING) {
PHALCON_THROW_EXCEPTION_STR(phalcon_mvc_view_exception_ce, "prefix must be a string");
@@ -3728,6 +3728,7 @@ PHP_METHOD(Phalcon_Mvc_View_Engine_Volt_Compiler, compile){
/**
* Compile always must be used only in the development stage
*/
+ PHALCON_INIT_VAR(compilation);
phalcon_call_method_p3(compilation, this_ptr, "compilefile", template_path, real_compiled_path, extends_mode);
} else {
if (PHALCON_IS_TRUE(stat)) {
or even diff --git a/ext/mvc/view/engine/volt/compiler.c b/ext/mvc/view/engine/volt/compiler.c
index 21c841a..f828389 100644
--- a/ext/mvc/view/engine/volt/compiler.c
+++ b/ext/mvc/view/engine/volt/compiler.c
@@ -3728,6 +3728,7 @@ PHP_METHOD(Phalcon_Mvc_View_Engine_Volt_Compiler, compile){
/**
* Compile always must be used only in the development stage
*/
+ PHALCON_INIT_NVAR(compilation);
phalcon_call_method_p3(compilation, this_ptr, "compilefile", template_path, real_compiled_path, extends_mode);
} else {
if (PHALCON_IS_TRUE(stat)) {But now I am a bit clueless regarding PHALCON_OBS_NVAR(): I see cases where it is used before phalcon_array_fetch_string() but also see cases where it is not. I must say I did not look closely int the memory management API though. What are the advantages of PHALCON_OBS_(N)VAR over PHALCON_INIT_(N)VAR? |
ghost
mentioned this pull request
|
PHALCON_OBS_VAR tracks a variable in the active memory frame, the variable is tracked without check if it was marked for tracking before. PHALCON_OBS_NVAR do the same but it checks if the variable is not pointing to NULL which means the variable is already tracked. |
|
Just trying to understand why
PHALCON_INIT_VAR(znull);
/* ... */
phalcon_update_property_this(this_ptr, SL("_blocks"), znull TSRMLS_CC);
/* ... */
PHALCON_CPY_WRT(prefix, znull);
/* ... */
PHALCON_CPY_WRT(compilation, znull);At this point If we remove At this point we will have Question: if This is what I am thinking of (quick and dirty): diff --git a/ext/kernel/fcall.c b/ext/kernel/fcall.c
index 1263f83..646cdb3 100755
--- a/ext/kernel/fcall.c
+++ b/ext/kernel/fcall.c
@@ -36,6 +36,10 @@
#include "kernel/alternative/fcall.h"
+#ifdef linux
+#include <execinfo.h>
+#endif
+
/**
* Finds the correct scope to execute the function
*/
@@ -306,6 +310,32 @@ static inline int phalcon_call_method_params_internal(zval *return_value, zval *
if (!noreturn) {
ALLOC_INIT_ZVAL(return_value);
}
+ else {
+ int ok = 1;
+ if (Z_REFCOUNT_P(return_value) > 1) {
+ zend_printf("phalcon_call_method_params_internal(): return_value has %d references, expect crashes!\n", Z_REFCOUNT_P(return_value));
+ ok = 0;
+ }
+ else if (Z_TYPE_P(return_value) > IS_BOOL) {
+ /* Destructors won't be called */
+ zend_printf("phalcon_call_method_params_internal(): return_value has type %d, expect memory leaks\n", Z_TYPE_P(return_value));
+ ok = 0;
+ }
+#ifdef linux
+ if (!ok) {
+ int i;
+ void* backtrace_buf[4096];
+ int stack_size = backtrace(backtrace_buf, sizeof(backtrace_buf)/sizeof(void*));
+ char** stack_symbols = backtrace_symbols(backtrace_buf, stack_size);
+
+ for (i=0; i<stack_size; ++i) {
+ zend_printf("#%d %p [%s]\n", i, backtrace_buf[i], stack_symbols[i]);
+ }
+
+ zend_printf("\n");
+ }
+#endif
+ }
active_scope = EG(scope);
This is how it will look like: What do you think (e.g., for 1.3.0)? |
|
Your explanation is correct, the patch looks great!, very useful, we can add an: #ifndef PHALCON_RELEASE to remove that on the build/ versions |
ghost
mentioned this pull request
|
I see That you have generated a backtrace and solution has been found. |
ghost
closed this
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix issues similar to #741
Please review carefully!