-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shutdown_executor() refactoring (reuse opcache fast request shutdown … #2591
Conversation
@nikic could you, please, review this. |
|
||
zend_try { | ||
zend_llist_destroy(&CG(open_files)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move the EG(active) = 0
to the top of this function instead of the bottom? We had issues in the past where objects could be created after destructors were already run. Setting active=0 early should make completely sure that no user code will run.
Zend/zend_hash.h
Outdated
} \ | ||
Z_NEXT(prev->val) = Z_NEXT(_p->val); \ | ||
} else { \ | ||
HT_HASH(__ht, _p->h | __ht->nTableMask) = Z_NEXT(_p->val); \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Stray whitespace after indentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also could write this as HT_HASH(__ht, nIndex) = Z_NEXT(_p->val)
, we already have the var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right
|
||
pefree(stream, stream->is_persistent); | ||
} | ||
#else |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this code dropped? Shouldn't it still work fine, as fast_shutdown is only used in non-debug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we closed resources after EG(symbol_table) destruction, now we close resources and free objects before (in both RELEASE and DEBUG builds)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, that makes sense. I guess dropping this is okay, these checks did not work particular well anyway.
I did not see any specific problems, but also not really sure about the possible interactions. We'll probably only find out after trying it... I heard previously that opcache.fast_shutdown had stability issues. What caused them and how are they resolved by this patch (if they existed at all)? |
I tried to follow the standard executor_shutdown() behavior. Just injected code from opcache. Then (after moving resource and object destruction before symbol_table destruction), I found that some part of old logic (separate static variables destruction) became useless. I hope, this shouldn't make big problems. Thanks for review. |
Can we get a ini setting back for this? I believe we have stability issues with the fast shutdown. Specifically, segfaults when GC'ing a complex shutdown function. |
It would be great to catch and fix the real problem of your crash. |
@dstogov I've opened a bug and I think you've worked quite a lot in the particular areas. Would you mind taking a look at the traces or if there's anything else we can provide to help debug? |
No description provided.