Skip to content
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

Develop #3

Closed
wants to merge 3 commits into from
Closed

Develop #3

wants to merge 3 commits into from

Conversation

albertcht
Copy link
Contributor

@albertcht albertcht commented Nov 11, 2017

Hi Huang-Yi,

First, thanks a lot for your amazing package and high code quality! I did some change in 2 places.

  1. I found when I tried to do some benchmark testing, sometimes it threw a 'connection[...] is closed' error. So I checked response fd before sending response to prevent this error.
  2. After Lumen 5.2, the application instance didn't implement ApplicationContract. So I let it check the Container instance to prevent a memory leak issue in Lumen.

Best Regards,
Albert Chen.

@huang-yi
Copy link
Owner

Hi,@albertcht

看了你的资料,我想我用简体中文和你对话应该没有问题,因为我的英文表达不是很流利,希望你不要介意。

首先感谢你的贡献!

关于第1点,我重新做大量了测试,发现如果client提前关闭与server的连接,就会导致这个错误,但是使用swoole_server::exist方法来做检测也不能完全避免这个问题,因为我们无法预测client的关闭时机,很有可能在做exist检测的时候client还未与server断开连接,所以依然会触发上面的error。

第2点的改进非常棒,你能不能为这个commit创建一个独立的pull request?我希望先合并这个,因为第1点还需要一个更好的改善机制。

@albertcht
Copy link
Contributor Author

albertcht commented Nov 14, 2017

Hi @huang-yi ,

沒有問題的,第一點我後來也發現這樣的作法無法完全避免這樣的錯誤產生,不過他並不會導致 worker 掛掉重啟,所以應該還不會導致致命的錯誤。
關於第二點我會另外發起一個新的 pull request 請求給你。

另外想請問您之後是否對這個專案是否繼續有一些目標規劃呢?我這裡有意願長期對這個項目做維護開發,目前初步的想法大概是:

  1. 支援�使用者能自定義在 onRequest 結束後針對某些 ServiceProvider 進行重新註冊,以解決某些特定單例模式下所造成的變數污染問題。(不確定是否這樣做會比較好)
  2. 整合 Websocket Server。

也希望能聽取您的建議,這樣除了 BUG 修復外,若有你想要的相關 feature 的 commit 我也能一併給你發 pull request。

@huang-yi
Copy link
Owner

@albertcht

最初这个专案仅仅是应用于我自己的一些应用,新的feature都来自于实际需求。开源目的也是希望有更多的思想注入进来,所以我非常支持你的想法,若有好的feature欢迎提交给我。

关于你的规划:

第一点我也认为是很有必要的,所以我保留了Laravel的terminate机制。受你的启发,我觉得可以使用配置的形式来实现自定义,这样的使用体验应该会更好。

第二点热重载也是我一直在思考的问题,这的确是开发者的痛点,但是还未找到好的方案。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants