Skip to content

Avoid eval for GraalVM#136

Closed
borkdude wants to merge 5 commits into
funcool:masterfrom
borkdude:master
Closed

Avoid eval for GraalVM#136
borkdude wants to merge 5 commits into
funcool:masterfrom
borkdude:master

Conversation

@borkdude

@borkdude borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor

Promesa doesn't work on GraalVM due to runtime (non-top-level) usage of eval.
This PR avoids doing that and decides at compile time which branch should be taken.

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

@niwinz Unrelated to this PR: I noticed that promesa now always picks a virtual thread executor by default if it's available. Is this intentional? Are virtual thread absolutely always better or should the user still have a choice?

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Btw, I think the failing tests aren't due to this PR, but just flakiness in general.

@niwinz

niwinz commented Mar 9, 2023

Copy link
Copy Markdown
Member

@niwinz Unrelated to this PR: I noticed that promesa now always picks a virtual thread executor by default if it's available. Is this intentional? Are virtual thread absolutely always better or should the user still have a choice?

I'm not aware of it, can you show me an example? There are different thread constructors for different purposes and if the default one is a vthread it should be a bug.

@niwinz

niwinz commented Mar 9, 2023

Copy link
Copy Markdown
Member

Btw, I think the failing tests aren't due to this PR, but just flakiness in general.

yep, I'm aware of it, I need to take time to fix it properly, sometimes tests fails...

Comment thread src/promesa/exec.cljc Outdated
@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

@niwinz My bad about the default: didn't look closely enough, seems good. PR should be ready now unless you have more feedback.

@niwinz

niwinz commented Mar 9, 2023

Copy link
Copy Markdown
Member

@borkdude 👍 I will try fix the randomly failing test also

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Sorry, one failure now in JDK19 which I will fix.

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Sorry not done yet, the code still doesn't run well if you don't provide --enable-preview, I'll also fix this.

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Should be ok now.

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

Feel free to re-trigger Github Actions to have some more confidence that I didn't screw something up.

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

@niwinz Actually I'm not sure if spinning up a thread at compile time is something that GraalVM would like. I'll test this locally.

@niwinz

niwinz commented Mar 9, 2023

Copy link
Copy Markdown
Member

I have merged the current PR, but feel free to open a new one, maybe we can think in an other approach.
In any case (Thread/ofVirtual) does not spins a thread, it just returns a builder instance.

@niwinz niwinz closed this Mar 9, 2023
@niwinz

niwinz commented Mar 9, 2023

Copy link
Copy Markdown
Member

I merged it with command line, squahing your commits

@borkdude

borkdude commented Mar 9, 2023

Copy link
Copy Markdown
Contributor Author

In any case (Thread/ofVirtual) does not spins a thread, it just returns a builder instance.

Hah, well that's quite innocent I guess. Thanks.

@helins

helins commented Mar 9, 2023

Copy link
Copy Markdown

I tested this PR with my native image and I don't get any runtime errors anymore 👍

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

Successfully merging this pull request may close these issues.

3 participants