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

Allow Quasar to work with multiple classloaders. #209

Merged
merged 1 commit into from
Aug 6, 2016

Conversation

mikehearn
Copy link
Contributor

Hi Ron, Fabio,

This commit is the work of my colleague Matthew Nesbit. It upgrades Quasar so it can operate with multiple classloaders, which is useful if you wish to dynamically load fiber implementations from plugins, inside a sandbox etc. The unit tests all pass for me, and we have it working internally, but there may still be issues or a need to tweak things further.

@circlespainter
Copy link
Member

circlespainter commented Jun 22, 2016

Thanks a lot Matthew and Mike! We'll look into the PR soon, which could also close #196 [edit] when merged, right? If you have the time, some basic unit test(s) with multiple classloaders would really be great.

@mikehearn
Copy link
Contributor Author

@circlespainter I've pushed a new version from Matthew that has unit tests as well. How does it look?

@circlespainter
Copy link
Member

It looks good but the new tests seem to pass even on master (i.e. without the classloaders patch), shouldn't they pass only with the improvements instead?

@mikehearn
Copy link
Contributor Author

OK, so here's what I understand having chatted to Matthew about this some more: the current code in git master actually does work with multiple classloaders, but only kind of by accident, and the errors you get back when something goes wrong are incomprehensible as a result.

These patches add explicit tests and explicit tracking of classloaders, meaning that things work by design rather than by accident, and the results when something goes wrong are now better. At least, that's my high level understanding.

@circlespainter
Copy link
Member

I studied the PR and I think it contains important improvements but I also think that tests should identify them as precisely as possible and one important reason is that this allows to detect immediately when changes cause regressions in them.

If you know for sure that the previous code got into trouble and that it was hard to figure out what was happening, I think you've also seen that trouble already: can you recall/reconstruct it and tailor the tests to it, or just tell how to reproduce it? The only one I could identify so far (and write a test tailored to it) was extra unneeded instrumentation but that one isn't really supposed to cause any trouble, rather just a potential (and tiny, unless the triggering circumstance occurs everywhere all the time, which is unlikely) slowdown.

@pron pron merged commit a7f9a54 into puniverse:master Aug 6, 2016
@pron
Copy link
Contributor

pron commented Aug 6, 2016

I have merged this PR but made some refactoring to QuasarInstrumentor and MethodDatabase as a result. The instrumentor does not have a default DB now.

@mikehearn
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants