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

Fix incorrect event loop behavior on Windows #1367

Merged
merged 1 commit into from
Oct 18, 2017
Merged

Fix incorrect event loop behavior on Windows #1367

merged 1 commit into from
Oct 18, 2017

Conversation

kimushu
Copy link
Contributor

@kimushu kimushu commented Oct 18, 2017 •
edited
Loading

See #1363

This patch solves two problems on Windows.

Bug#1. Event loop stops even if we have pending writes (and reads).

  • Moved uv_async_init from WriteThread to NAN_METHOD(Write). This async handle will keep event loop alive.

Bug#2. Event loop does not stop even if all pending writes (and reads) has finished.

  • Added uv_close to EIO_AfterWrite. This removes async handle from event loop.

@kimushu kimushu mentioned this pull request Oct 18, 2017
Closed
@codecov-io
Copy link

Codecov Report

Merging #1367 into master will increase coverage by 0.14%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1367      +/-   ##
==========================================
+ Coverage   79.43%   79.58%   +0.14%     
==========================================
  Files          21       20       -1     
  Lines         919      862      -57     
  Branches      166      162       -4     
==========================================
- Hits          730      686      -44     
+ Misses        189      176      -13
Impacted Files Coverage Δ
lib/bindings/auto-detect.js 69.23% <0%> (-30.77%) ⬇️
lib/bindings/win32.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e4b1f9...69cfded. Read the comment docs.

@reconbot reconbot requested a review from dustmop October 18, 2017 16:56
reconbot
reconbot reviewed Oct 18, 2017
View reviewed changes
src/serialport_win.cpp
@@ -293,9 +299,13 @@ NAN_METHOD(Write) {
baton->offset = 0;
baton->callback.Reset(info[2].As<v8::Function>());
baton->complete = false;

uv_async_t* async = new uv_async_t;
Copy link
Member

Choose a reason for hiding this comment

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

👍

reconbot
reconbot approved these changes Oct 18, 2017
View reviewed changes
Copy link
Member

@reconbot reconbot left a comment

Choose a reason for hiding this comment

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

This looks good to me, I learn something new about libuv every day. I'll wait for @dustmop to review too.

@dustmop
Copy link
Contributor

dustmop commented Oct 18, 2017

LGTM, thanks for the fixes!

@reconbot reconbot merged commit c1d9d88 into serialport:master Oct 18, 2017
@kimushu kimushu mentioned this pull request Oct 24, 2017
Merged
@kimushu kimushu deleted the fix/read-write-async branch October 24, 2017 15:16
@lock lock bot locked and limited conversation to collaborators Apr 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@reconbot reconbot reconbot approved these changes

@dustmop dustmop Awaiting requested review from dustmop

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kimushu @codecov-io @dustmop @reconbot