Skip to content
This repository has been archived by the owner on Oct 30, 2021. It is now read-only.

Handle potential exception in mergeQueue #56

Merged
merged 6 commits into from
Sep 29, 2016
Merged

Conversation

springmeyer
Copy link
Contributor

In all cases where C++ exceptions are possible we should use try/catch to ensure the C++ exception is sent back to JS land as a JS Error. C++ functions in the threadpool should use try/catch and then set a property of the baton to pass the error back to the main thread.

Followups needed:

  • coalesceSingle and coalesceMulti are also missing try/catch

/cc @sbma44 @yhahn @mapsam

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 95.519% when pulling fbadd77 on handle-threadpool-exception into 555578f on master.

@sbma44
Copy link
Member

sbma44 commented Sep 28, 2016

ok! I think that should do it. would love 👀 @springmeyer. Not sure how to add tests for this, though -- there are various assert.throws() around coalesce but that's because the entry method already takes pains to sanitize input before handing things to the threadpool. I am not currently having problems with coalesce so I'm left scratching my head how to slip past those checks and cause problems that the new code would handle.

@springmeyer
Copy link
Contributor Author

Looks good @sbma44. Right, I don't think there is an easy way to trigger a throw inside the coalesce functions. There is definitely the possibility (structures from the C++ stdlib are being used which hypothetically could throw under low memory) but not obvious to trigger. So in the case of coalesce I would not worry about a test.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 94.737% when pulling ed07d71 on handle-threadpool-exception into 555578f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 95.147% when pulling a583c59 on handle-threadpool-exception into 555578f on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 95.147% when pulling b33a17b on handle-threadpool-exception into 555578f on master.

@sbma44 sbma44 merged commit 37a28a5 into master Sep 29, 2016
@sbma44 sbma44 deleted the handle-threadpool-exception branch September 29, 2016 17:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants