fix: propagate async context in callbacks #1765
Conversation
Codecov Report
@@ 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
|
|
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? |
|
You can make the commit bigger to clean up the code. No worries. |
|
I think requiring a higher version of gcc would be a major version change and I'd like to avoid it. |
|
(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? |
|
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 If the non-static data member initializers for strings are replaced with code in the constructor I think it will work. |
|
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] = ""; |
There was a problem hiding this comment.
Why is this one ok when the others weren't? Because it's for windows?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
They're equivalent. Just pushed so they're consistent.
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.
AsyncResourceneeds to be constructed with aconst char*name unique to each of yourstructs (so can't inherit from some other class that extendsAsyncResource). Constructing theAsyncResource(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