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.

Sign up for GitHub

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

Jump to bottom

feat: close return promise #128

Merged
merged 1 commit into from
Oct 26, 2016
Merged

feat: close return promise #128

merged 1 commit into from
Oct 26, 2016

Conversation

popomore
Copy link
Member

@popomore popomore commented Oct 24, 2016 •
edited
Loading

Checklist
  • npm test passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

close

Description of change

logger should close stream when app close

@mention-bot
Copy link

@popomore, thanks for your PR! By analyzing the history of the files in this pull request, we identified @fengmk2 and @dead-horse to be potential reviewers.

popomore
popomore commented Oct 24, 2016
View reviewed changes
lib/egg.js
return super
.close()
.then(() => {
for (const logger of this.loggers.values()) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

windows 可能跟这个有关

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.values() 在 node 4.x 就支持了?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是 Map 的方法

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

喔噢

@dead-horse
Copy link
Member

CI 挂了？

atian25
atian25 reviewed Oct 24, 2016
View reviewed changes
lib/agent.js
return super
.close()
.then(() => {
process.removeListener('uncaughtException', this._uncaughtExceptionHandler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

失败后不用 remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是指哪里失败？

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 promise 不用 catch？

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

使用的时候 catch

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个我改下

lib/egg.js
return super
.close()
.then(() => {
for (const logger of this.loggers.values()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.values() 在 node 4.x 就支持了?

@popomore
Copy link
Member Author

有个问题还要修下

@popomore
Copy link
Member Author

是因为使用了 node-homedir，无法用 env 来 mock HOME

@popomore
Copy link
Member Author

eggjs/egg-core#25

@codecov-io
Copy link

codecov-io commented Oct 24, 2016 •
edited
Loading

Current coverage is 95.74% (diff: 100%)

Merging #128 into master will increase coverage by 0.13%

@@             master       #128   diff @@
==========================================
  Files            33         33          
  Lines           821        823     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            785        788     +3   
+ Misses           36         35     -1   
  Partials          0          0

Powered by Codecov. Last update 03d70e1...c7b7939

@popomore popomore force-pushed the close branch 9 times, most recently from 5a2dc3f to fb537b0 Compare October 24, 2016 17:02
@popomore
Copy link
Member Author

ci 经常挂掉， cnpm/npminstall#152

@popomore
Copy link
Member Author

可能跟这个有关 isaacs/rimraf#72

@popomore popomore force-pushed the close branch 3 times, most recently from 48179ba to d0eeb90 Compare October 24, 2016 18:29
@popomore
Copy link
Member Author

终于有一次跑过了 https://ci.appveyor.com/project/eggjs/egg/build/1.0.147/job/f0y45tplamojdlnv

@popomore popomore changed the title feat: close return promise WIP: feat: close return promise Oct 24, 2016
@popomore
Copy link
Member Author

难道跑过，先等着合影

@fengmk2
Copy link
Member

fengmk2 commented Oct 25, 2016

+1 不容易啊，原来 windows 要求进程打开的文件句柄被关闭了才能删除文件，果然是传说中 windows 的文件 io 做得比较好。。

@fengmk2 fengmk2 added the core label Oct 25, 2016
@fengmk2 fengmk2 added this to the v1.x milestone Oct 25, 2016
@fengmk2 fengmk2 mentioned this pull request Oct 25, 2016
Merged
4 tasks
popomore
popomore commented Oct 25, 2016
View reviewed changes
test/app/middleware/site_file.test.js
@@ -47,7 +48,8 @@ describe('test/app/middleware/site_file.test.js', () => {
return request(app.callback())
.head('/robots.txt')
.expect('content-length', '72')
.expect('') // body must be empty for HEAD
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个不知道哪里改了

@popomore
feat: close return promise
af3ca64 
logger should close stream when app close
@popomore
Copy link
Member Author 

SyntaxError: Unexpected end of JSON input
    at Object.parse (native)
    at C:\projects\egg\node_modules\.0.4.5@istanbul\lib\command\report.js:110:44
    at Array.forEach (native)
    at C:\projects\egg\node_modules\.0.4.5@istanbul\lib\command\report.js:109:19
    at C:\projects\egg\node_modules\.0.4.5@istanbul\lib\util\file-matcher.js:39:20
    at f (C:\projects\egg\node_modules\.1.4.0@once\once.js:25:25)
    at Glob.<anonymous> (C:\projects\egg\node_modules\.5.0.15@glob\glob.js:133:7)
    at emitOne (events.js:96:13)
    at Glob.emit (events.js:188:7)
    at Glob._finish (C:\projects\egg\node_modules\.5.0.15@glob\glob.js:172:8)
[common-bin] run cov with [] at C:\projects\egg error:
Error: C:\projects\egg\node_modules\.0.4.5@istanbul\lib\cli.js report,--root,C:\projects\egg\coverage,text-summary,json,lcov exit with code 1
    at ChildProcess.<anonymous> (C:\projects\egg\node_modules\.1.0.0@common-bin\lib\helper.js:31:21)
    at ChildProcess.g (events.js:291:16)
    at emitTwo (events.js:106:13)
    at ChildProcess.emit (events.js:191:7)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
npm ERR! Windows_NT 6.3.9600

部分情况会有这个问题

@popomore
Copy link
Member Author

不是很稳定，把延时时间再调长一点。

@popomore
Copy link
Member Author

Windows 的 report 是不是也要等一下

@popomore
Copy link
Member Author

eggjs/egg-bin#16

@popomore popomore changed the title WIP: feat: close return promise feat: close return promise Oct 25, 2016
@popomore
Copy link
Member Author

这个差不多先合好了，不稳定的之后看看，合了才能改 mock。

@dead-horse
Copy link
Member

+1

@dead-horse dead-horse merged commit 5dafb58 into master Oct 26, 2016
@dead-horse dead-horse deleted the close branch October 26, 2016 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atian25 atian25 atian25 left review comments

Assignees

@dead-horse dead-horse

Labels
core
Projects
None yet
Milestone
1.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@popomore @mention-bot @dead-horse @codecov-io @fengmk2 @atian25