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

fix: propagate async context in callbacks #1765

Merged
merged 1 commit into from
Jan 6, 2019

Conversation

zbjornson
Copy link
Contributor

Fixes #1751

I've tested this on Windows with real hardware (repro'ed the issue in #1751). I can't get WSL to work with a serial port right now to test on Ubuntu.

I can't think of a good way to make this commit smaller. AsyncResource needs to be constructed with a const char* name unique to each of your structs (so can't inherit from some other class that extends AsyncResource). Constructing the AsyncResource (in a ctor or initializer list) makes it necessary to initialize everything else that you were relying on value-initialization for. Rather than only initializing the members that you're not setting where the structs are used, I went with the safer option of initializing (almost) everything explicitly. Using more C++ features (std::strings) would shorten the code overall (possibly making initialization unnecessary) but would make this commit even bigger. /ramble

@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

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

@@            Coverage Diff            @@
##           master   #1765      +/-   ##
=========================================
+ Coverage   63.55%   65.5%   +1.95%     
=========================================
  Files          22      21       -1     
  Lines         845     803      -42     
=========================================
- Hits          537     526      -11     
+ Misses        308     277      -31
Impacted Files Coverage Δ
packages/bindings/lib/index.js 70% <0%> (-30%) ⬇️
packages/bindings/lib/darwin.js

@zbjornson
Copy link
Contributor Author

Urgh, those CI failures are due to a GCC bug that's fixed in 4.10.0, I think (see https://travis-ci.org/node-serialport/node-serialport/builds/475554563, which used GCC5 and was successful).

I can't figure out how NAN's AsyncResource test, which uses the same syntax, builds with 4.8.5 (e.g. https://travis-ci.com/nodejs/nan/jobs/166028162). Anyone see something I'm missing?

@reconbot
Copy link
Member

reconbot commented Jan 5, 2019

You can make the commit bigger to clean up the code. No worries.

@reconbot
Copy link
Member

reconbot commented Jan 5, 2019

I think requiring a higher version of gcc would be a major version change and I'd like to avoid it.

@zbjornson
Copy link
Contributor Author

(Let's figure out the compiler issue before I make other changes.)

A last resort (aside from upgrading gcc) might be to implement our own version of AsyncResource that doesn't need this sort of initialization that makes GCC croak.

@fivdi do you have any ideas why this fails to compile with old GCC, but Nan's test works?

@fivdi
Copy link
Contributor

fivdi commented Jan 5, 2019

It looks like the compile error messages are pointing at the wrong lines of code and that it's actually the non-static data member initializers for strings like this one or this one that are causing the errors.

There's an online compiler for doing some tests with gcc 4.8, for example, this doesn't work and shows the compile error "array used as initializer", but this works and displays "all ok".

If the non-static data member initializers for strings are replaced with code in the constructor I think it will work.

@zbjornson
Copy link
Contributor Author

Excellent find, thank you! GCC 5 with -std=c++03 (since that requires c++11) shows the correct error message. Will move that initializer.

Nan::Callback callback;
std::list<ListResultItem*> results;
char errorString[ERROR_STRING_SIZE];
char errorString[ERROR_STRING_SIZE] = "";
Copy link
Member

Choose a reason for hiding this comment

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

Why is this one ok when the others weren't? Because it's for windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It's valid c++11 and the oldest version of MSVC that Node.js builds with is fine with it. If you want them to be consistent I'll happily change it.

Copy link
Member

Choose a reason for hiding this comment

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

I am uncertain what a uninitialized char array looks like, so I don't know if this causes any different behaviors between the two different ways in how we use it. If it's just aesthetic then sure let's make it consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're equivalent. Just pushed so they're consistent.

Copy link
Member

Choose a reason for hiding this comment

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

ace! I'll merge!

@reconbot reconbot merged commit 9b5dbdb into serialport:master Jan 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

receiving data delayed / loop issues
3 participants