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

Callbacks in class must be promises #20

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Julusian
Copy link
Collaborator

@nytamin you are going to love this 'fun' case that I've spent quite a few hours today digging into today.

Essentially, as it is right now, any callback passed into a ThreadedClass gets slightly mangled, so that it returns a promise.
If the class does not define in its interface that the callback should return a promise, this will lead to weird type issues, as it will be getting a promise anyway. eg testcase class normal-callback gets back a value of "[object Promise]5" instead of the expected 15.

This becomes quite a problem when the class is an EventEmitter, as it means that when the the parent does an .on on the threadedClass, if it throws an error in the callback provided to .on, then that error gets lost as an UnhandledPromiseRejection.
The expected behaviour here is for the error to get bubbled back to the emit.

I'm not sure how a solution to this will work. Somehow the callbacks passed in need to block the thread execution (fibers?)

@nytamin
Copy link
Owner

nytamin commented Oct 23, 2019

You're right, this is a real issue, and one that probably points out a fundamental flaw in the threadedClass.
Fixing it is probably not straightforward, perhaps fibers would be a solution as you suggest, but that might be a big refactoring to do.

I'm putting this on hold for now due to lack of time/resources, but if anyone would want to look into this, I'm all for merging it in.

@nytamin nytamin added help wanted Extra attention is needed bug Something isn't working labels Oct 23, 2019
@Julusian Julusian mentioned this pull request Jun 20, 2020
4 tasks
@Julusian Julusian marked this pull request as draft December 17, 2021 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants