Nk0273926 patch 1 - #13632
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| port: System::getEnv('PORT', 80), | ||
| settings: [ | ||
| Constant::OPTION_WORKER_NUM => $totalWorkers, | ||
| Constant::OPTION_HOOK_FLAGS => SWOOLE_HOOK_ALL, |
There was a problem hiding this comment.
This runtime-wide hook change is intended to resolve a concurrency deadlock, but the PR adds no observable concurrency test. Because SWOOLE_HOOK_ALL changes blocking I/O behavior across every HTTP request, please exercise parallel synchronous executions and assert that both complete without deadlocking. A test that merely inspects this configuration value would not validate the fix.
Knowledge Base Used: Application bootstrap and runtime lifecycle
Prompt To Fix With AI
This is a comment left during a code review.
Path: app/http.php
Line: 63
Comment:
**Concurrency behavior remains untested**
This runtime-wide hook change is intended to resolve a concurrency deadlock, but the PR adds no observable concurrency test. Because `SWOOLE_HOOK_ALL` changes blocking I/O behavior across every HTTP request, please exercise parallel synchronous executions and assert that both complete without deadlocking. A test that merely inspects this configuration value would not validate the fix.
**Knowledge Base Used:** [Application bootstrap and runtime lifecycle](https://app.greptile.com/appwrite/-/custom-context/knowledge-base/appwrite/appwrite/-/docs/application-bootstrap.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| $database = $input['_APP_DB_ADAPTER'] ?? 'postgresql'; | ||
| $name = $account['name'] ?? 'Admin'; | ||
| $email = $account['email'] ?? 'admin@selfhosted.local'; | ||
|
|
There was a problem hiding this comment.
Removing name and email from telemetry leaves the entire $account argument unused here, while both callers still pass it and the coroutine closure still captures it. This needlessly extends the lifetime of account data, including the installer password. Remove the parameter and its call-site captures now that telemetry no longer consumes it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Platform/Tasks/Install.php
Line: 878
Comment:
**Account data remains captured**
Removing `name` and `email` from telemetry leaves the entire `$account` argument unused here, while both callers still pass it and the coroutine closure still captures it. This needlessly extends the lifetime of account data, including the installer password. Remove the parameter and its call-site captures now that telemetry no longer consumes it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.1 participant
What does this PR do?
This PR adds SWOOLE_HOOK_ALL to the Swoole HTTP server settings.
This is intended to address the synchronous function execution deadlock
described in issue #13485.
I have not yet verified the fix with a concurrency test. The existing
custom dispatch function and SWOOLE_DISPATCH_UIDMOD setting are unchanged.
Test Plan
Constant::OPTION_HOOK_FLAGS => SWOOLE_HOOK_ALLto the Swoole HTTP server settings.SWOOLE_DISPATCH_UIDMODsetting are unchanged.Related PRs and Issues
Checklist