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

Fix state exec not working when optin.unstable.async_runtime is enabled. #3332

Closed
wants to merge 2 commits into from

Conversation

mitchell-as
Copy link
Contributor

@mitchell-as mitchell-as commented May 29, 2024

BugDX-2868 `state exec` throws nil pointer error when `optin.unstable.async_runtime` is enabled.

@mitchell-as mitchell-as requested a review from MDrakos May 29, 2024 21:09
@mitchell-as mitchell-as marked this pull request as ready for review May 29, 2024 21:09
Copy link
Member

@Naatan Naatan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue here is that async is not returning a runtime.

Async = don't source runtime updates. It does not mean "don't even initialize a runtime".

The bug here is taht async is returning nil. It should return the runtime as-is, without doing any update.

I don't think overrideAsyncTriggers should exist at all.

@Naatan
Copy link
Member

Naatan commented May 30, 2024

btw I fixed this in my runtime refactor, but probably good to land this independently as well in case the refactor takes much longer.

@mitchell-as
Copy link
Contributor Author

mitchell-as commented May 30, 2024

@Naatan Well I need a runtime update for my test in #3331, and this is the smallest change that makes it work. Since you've already refactored this, is it worth me trying to refactor it now? I don't even understand the whole async thing, let alone why overrideAsyncTriggers should or should not exist. I didn't write the original code. I only went through the debugger to figure out why it wasn't working for state exec.

If you'd like me to make changes, please be more specific. I don't want to burn more time guessing what you mean.

@Naatan
Copy link
Member

Naatan commented May 30, 2024

I believe what should happen is:

  • Move the async conditional on line 76 to right after the runtime constructed (line 82)
  • Drop overrideAsyncTriggersMap

Sorry if I'm not being clear, I thought I was but it can sometimes be hard to have the right perspective.

Basically when we request a runtime we do two things; we retrieve the runtime as it exists on disk, and then if we find that state to be outdated with the commit ID requested we update it, and then return the updated runtime.

What async tells us is to not go through that update logic. Just return the runtime as it exists on disk. It does not mean "don't even give me a runtime".

@mitchell-as
Copy link
Contributor Author

That didn't work. I'm getting a weird error when running state exec:

Operating on project ActiveState-CLI/Perl-5.36, located at /tmp/3916057409/work.                                                            
                                                                                                                                                      
          █ Something Went Wrong                                                                                                                      
                                                                                                                                                      
           x Could not retrieve environment information for your runtime.                                                                             
           x Your installation seems corrupted.                                                                                                       
            Please try to re-run this command, as it may fix the problem.  If the problem persists, please report it in our forum:                    
          [https://community.activestate.com/c/state-tool/.](https://community.activestate.com/c/state-tool/)                                                                                            
           x Your installation seems to be corrupted.                                                                                                 
                                                                                                                                                      
            Cannot find runtime definition file at /tmp/3916057409/cache/8d7d4588/_runtime_store/runtime.json.                                        
            You can try to manually delete the cache directory and re-activating the project.

I'm reluctant to spend time trying to debug and develop this further considering it will be throwaway code.

@mitchell-as
Copy link
Contributor Author

I'm going to close this. I only opened it so that we could track that state exec was broken for async runtimes. Since an upcoming runtime refactor will fix this, I think this PR is pointless.

@Naatan
Copy link
Member

Naatan commented May 31, 2024

Ah if it's only for async runtimes then yeah no harm waiting.

@mitchell-as mitchell-as reopened this Jul 9, 2024
@mitchell-as mitchell-as closed this Jul 9, 2024
@mitchell-as mitchell-as deleted the mitchell/dx-2868 branch July 9, 2024 20:10
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.

2 participants