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 access to V8Locker.thread and improve "invalid thread access" messages #228

Closed
drywolf opened this issue Feb 7, 2017 · 4 comments
Closed

Comments

@drywolf
Copy link
Contributor

drywolf commented Feb 7, 2017

Hi,

I am trying to use J2V8 from within a server platform (SonarQube) which makes much use of multi-threading. Therefore it is quite a tricky thing to not violate the single threading access when interacting with J2V8.

To make it easier to reason about what is going on when using the V8Locker class to acquire and release access of a thread to a V8 runtime, I would propose:

  • to allow read access to the V8Locker.thread field that stores the currently assigned thread
  • improve the message of acquire() to tell the developer that acquisition of the locker failed and which threads are involved (V8Locker thread / thread requesting lock acquisition)
  • improve the message of checkThread() to tell the developer that the locker ownership-check failed and which threads are involved (V8Locker thread / thread checking lock ownership)
  • provide non-throwing versions of the above two methods for use in regular program-logic rather than the exception case

Please let me know what you think of those, it would really help me out to get things working much more effortlessly and quickly for what I'm trying.

I would provide a pull request once you can confirm that my proposed additions are a good idea.

Regards

@caer
Copy link
Contributor

caer commented Feb 8, 2017

A pull request that's currently pending a merge partially addresses this with the introduction of a ConcurrentV8 class (which you can see here).

Maybe the ConcurrentV8 functionality could be amended by or merged with your suggestions, or even better it may meet your needs entirely. ;D

@drywolf
Copy link
Contributor Author

drywolf commented Feb 8, 2017

Thanks for the reply.

I had a quick look at the PR and saw that it is not only about threading but also about JAVA<->JS interop with classes ... which is exactly what I'm trying to do currently 😄

I also saw in the PR discussion that J2V8-classes was mentioned which is what I am currently using and fixing/extending as I go along.

@irbull @mizumi @yofreke @drywolf
I think it would be great to have a joined discussion of what the supported features / usecases for "JAVA<-> JS interop + Multi-Thread access" should be and how J2V8 could best support them, and also to consolidate the work that is being done.

For example I integrated some missing / incomplete functionality in J2V8-classes that should allow to instantiate classes via Java reflection that were extened in a JS script from a base java class/interface.

Let me know what you guys are thinking, I would be happy to collaborate on this.

@caer
Copy link
Contributor

caer commented Feb 8, 2017

Glad it helped you! Yeah, a lot of people have been trying to do similar things across the board. The only thing my PR doesn't address is extending Java classes in JS.

However, it does support the implementation of Java functional interfaces (lambdas) with zero extra effort on either end of the API via the use of proxy classes. While this is definitely my own opinion showing through (and not in any way a fact), I'm of the belief that's it's better to avoid extending entire classes from Ecma-like script run times since it simplifies the architecture of the system, as well as avoids egregious hacks in the JS side.

That said, I think it would be trivial to add the capability for entire class extension to meet everyone's needs. I'd be delighted to have a discussion about these features so we can get them incorporated directly into V8.

@irbull
Copy link
Member

irbull commented May 25, 2017

Thanks everyone.

I have added some of these to master. In particular:

  1. I have provided access to the underlying thread field with dc82323
  2. I have improved the thread access error message with ffaced3
  3. I have implemented a tryAcquire that does not throw an exception, but rather returns true or false depending on if the lock could be acquired with 53981c1

We already have hasLock so I don't think we need a non-throwing version of checkThread. I'm going to close this issue. @drywolf let me know if these work for you, or if you think we need anything else on the V8Locker.

@irbull irbull closed this as completed May 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants