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

State and Contexts are not preserved through the Polyglot boundary #7117

Closed
radeusgd opened this issue Jun 23, 2023 · 5 comments · Fixed by #12233
Closed

State and Contexts are not preserved through the Polyglot boundary #7117

radeusgd opened this issue Jun 23, 2023 · 5 comments · Fixed by #12233
Assignees
Labels

Comments

@radeusgd
Copy link
Member

Whenever I call an Enso callback from within a polyglot call (i.e. I pass an Enso callback to a Java code as Function<A,B> and then execute that function in the Java code), the data about the State and more importantly Contexts is lost.

Repro

Create a new project, with a file polyglot/java/foo/foo/Foo.java:

package foo;

import java.util.function.Function;
import org.graalvm.polyglot.Value;

public class Foo {
    public static Value bar(Function<Integer, Value> f) {
        Value result = f.apply(42);
        return result;
    }
}

then in polyglot/java/foo run javac foo/Foo.java to get polyglot/java/foo/foo/Foo.class.

Now modify src/Main.enso to be:

from Standard.Base import all
import Standard.Base.Runtime.Context
import Standard.Base.Runtime.State

polyglot java import foo.Foo

run_inside_polyglot_callback ~action =
    callback _ = 
        action
    Foo.bar callback

report_states =
    IO.println "Context.Output.is_enabled = "+Context.Output.is_enabled.to_text
    state = Panic.catch Any (State.get Integer).to_text caught_panic->
        caught_panic.payload.to_display_text
    IO.println "State.get Integer = "+state

main =
    State.run Integer 123 <|
        IO.println "Direct:"
        report_states
        IO.println '\n'

        IO.println "Inside polyglot callback:"
        run_inside_polyglot_callback <| report_states
        IO.println '\n'

        IO.println '---------\n'
        IO.println "Now disabling context."
        Context.Output.with_disabled <|
            IO.println "Direct:"
            report_states
            IO.println '\n'

            IO.println "Inside polyglot callback:"
            run_inside_polyglot_callback <| report_states
            IO.println '\n'

and run the project.

Expected results

Direct:
Context.Output.is_enabled = True
State.get Integer = 123


Inside polyglot callback:
Context.Output.is_enabled = True
State.get Integer = 123


---------

Now disabling context.
Direct:
Context.Output.is_enabled = False
State.get Integer = 123


Inside polyglot callback:
Context.Output.is_enabled = False
State.get Integer = 123

Actual results

Direct:
Context.Output.is_enabled = True
State.get Integer = 123


Inside polyglot callback:
Context.Output.is_enabled = True
State.get Integer = State is not initialized for type Integer.


---------

Now disabling context.
Direct:
Context.Output.is_enabled = False
State.get Integer = 123


Inside polyglot callback:
Context.Output.is_enabled = True
State.get Integer = State is not initialized for type Integer.

We can see that inside of the callbacks that were executed by a Java call both the State and output context are lost - the state is treated as uninitialized and the Output context gets 're-enabled'.

I don't think this is correct.

@radeusgd
Copy link
Member Author

I'm implementing some synchronization primitives in #7072 using Java and callbacks and then inside of such synchronized methods the execution context information gets reset, leading to invalid results in my computations.

I will add workarounds for this, but I think the current behaviour is incorrect and not intuitive.

If we have to keep it, we at least need to very carefully document this oddity somewhere.

radeusgd added a commit that referenced this issue Jun 23, 2023
@radeusgd radeusgd mentioned this issue Jun 23, 2023
2 tasks
@JaroslavTulach
Copy link
Member

JaroslavTulach commented Jun 26, 2023

Currently State is passed as an argument throughout the Enso interpret execution. Such argument is obviously lost whenever a callback to and from polyglot call is made. To fix that we would need to keep state as thread local - either as a value in EnsoContext or as real ThreadLocal variable.

We should use ContextThreadLocal to store the State per EnsoContext and per thread.

radeusgd added a commit that referenced this issue Jun 26, 2023
@hubertp hubertp removed the triage label Jun 26, 2023
radeusgd added a commit that referenced this issue Jun 26, 2023
@jdunkerley jdunkerley moved this from ❓New to 📤 Backlog in Issues Board Jun 27, 2023
radeusgd added a commit that referenced this issue Jun 27, 2023
radeusgd added a commit that referenced this issue Jun 27, 2023
radeusgd added a commit that referenced this issue Dec 9, 2023
radeusgd added a commit that referenced this issue Dec 11, 2023
radeusgd added a commit that referenced this issue Dec 12, 2023
radeusgd added a commit that referenced this issue Dec 14, 2023
radeusgd added a commit that referenced this issue Dec 15, 2023
@radeusgd
Copy link
Member Author

Bumping this issue as I ran into it again when working on #10609 - I was surprised why a State read was not finding the value that I was sure is there, and I found out it was because of this bug.

radeusgd added a commit that referenced this issue Jul 31, 2024
radeusgd added a commit that referenced this issue Aug 1, 2024
radeusgd added a commit that referenced this issue Aug 2, 2024
mergify bot pushed a commit that referenced this issue Jan 22, 2025
Running some tests I noticed logs like
```
sty 21, 2025 10:12:30 PM org.enso.database.dryrun.OperationSynchronizer runMaintenanceActionIfPossible
SEVERE: A maintenance action failed with exception: As writing is disabled, cannot execute an update query. Press the Write button ▶ to perform the operation.
```

They appear rather randomly. This PR should provide a workaround until #7117 is fixed.
@JaroslavTulach JaroslavTulach moved this from 📤 Backlog to 🔧 Implementation in Issues Board Feb 5, 2025
@JaroslavTulach JaroslavTulach moved this from 🔧 Implementation to 👁️ Code review in Issues Board Feb 6, 2025
@enso-bot
Copy link

enso-bot bot commented Feb 17, 2025

Jaroslav Tulach reports a new STANDUP for yesterday (2025-02-16):

Progress: .

@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board Feb 17, 2025
@enso-bot
Copy link

enso-bot bot commented Feb 18, 2025

Jaroslav Tulach reports a new STANDUP for yesterday (2025-02-17):

Progress: .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: 🟢 Accepted
Development

Successfully merging a pull request may close this issue.

3 participants