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

Exceptions in pure code #172

Closed
ford-prefect opened this issue May 1, 2019 · 5 comments
Closed

Exceptions in pure code #172

ford-prefect opened this issue May 1, 2019 · 5 comments

Comments

@ford-prefect
Copy link
Contributor

Currently, if we have bad code that throws an exception despite being annotated as being pure, it seems to put Aff into a bad state where no more callbacks occur. I've written up an example at:

https://github.com/ford-prefect/purescript-sandbox

This is of course just bad code, but it might make sense to be defensive against this if there isn't a significant cost, as there are cases with a significant surface of JS interop, which can make for more common programming errors.
@ford-prefect
Copy link
Contributor Author

@natefaubion you mentioned performance concerns w.r.t. adding try/catches to deal with such programming errors. Is this something you've quantified (or that we could try to)? Might help make a more informed decision about the cost.

@natefaubion
Copy link
Collaborator

natefaubion commented May 10, 2019 •
edited
Loading

There are some basic benchmarks in the test directory for pure binds. I would run it as is, apply a try/catch around the STEP_BIND case in the interpreter (where we apply the continuation), and then see what the difference is. If there is a significant performance degradation, then I would try lifting the try/catch out of the main loop like we do for runSync and benchmark that.

@ford-prefect ford-prefect mentioned this issue May 13, 2019
Merged
@ford-prefect
Copy link
Contributor Author

Thanks @natefaubion -- I don't see a perf impact in the benchmark. pulp test takes ~4.8s to run with and without that change. I've made a PR for this (feel free to modify or ignore if there is a better way to do it, or I can do that too if you like).

@reactormonk
Copy link

This bug just cost me two days, so I wouldn't mind it being merged.

natefaubion pushed a commit that referenced this issue Aug 15, 2019
@ford-prefect @natefaubion
Try to recover from effectful code that is annotated as pure (#173)
7f9e362 
This puts a try-catch around the bind step in order to try to catch code
that is not expected to have effects but does (by throwing).

While this is clearly a programming error, it is certainly not
implausible that effects creep into FFI code inadvertently for Aff
users. This allows us to be defensive in that case and recover, rather
than have all asynchronous options quitely not work after.

The addition of the try-catch does not seem to have a performance impact
(tested on Node v10.15.3). With and without this change, `pulp test`
takes ~4.8s to run.

There is a discussion around this on #172.
@ford-prefect
Copy link
Contributor Author

Closed by #173

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@reactormonk @natefaubion @ford-prefect