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

Project folder renaming stopped working #6515

Closed
2 tasks
xvcgreg opened this issue May 3, 2023 · 19 comments · Fixed by #6665
Closed
2 tasks

Project folder renaming stopped working #6515

xvcgreg opened this issue May 3, 2023 · 19 comments · Fixed by #6665
Assignees
Labels
--bug Type: bug --regression Important: regression -language-server

Comments

@xvcgreg
Copy link

xvcgreg commented May 3, 2023

Discord username

No response

What type of issue is this?

Permanent – Occurring repeatably

Is this issue blocking you from using Enso?

  • Yes, I can't use Enso because of this issue.

Is this a regression?

  • Yes, previous version of Enso did not have this issue.

What issue are you facing?

Renaming of folders stopped working:
image

Expected behaviour

Folder to be named after the project name

How we can reproduce it?

No response

Screenshots or screencasts

No response

Enso Version

2023.5.2 nightly

Browser or standalone distribution

Standalone distribution

Browser Version or standalone distribution

standalone

Operating System

Windows

Operating System Version

Win11pro 22H2 22621.1555

Hardware you are using

12th Gen Intel(R) Core(TM) i9-12900HK / RTX3060 Laptop / Nvidia Drivers 531.68

@xvcgreg xvcgreg added --bug Type: bug --regression Important: regression triage labels May 3, 2023
@github-project-automation github-project-automation bot moved this to ❓New in Issues Board May 3, 2023
@4e6
Copy link
Contributor

4e6 commented May 3, 2023

@xvcgreg the directory rename is happening after the IDE is closed because we cannot move an opened project. Can you confirm that the folder is not renamed after the IDE is shut down?

@4e6 4e6 added s-info-needed Status: more information needed from submitter -language-server labels May 3, 2023
@4e6 4e6 self-assigned this May 3, 2023
@4e6 4e6 removed the triage label May 3, 2023
@xvcgreg
Copy link
Author

xvcgreg commented May 3, 2023

@4e6 yes, it was a project I created on Monday. But only today when I wanted to zip it I found out that it's not renamed. And yes, I have closed Enso in the mean time ;)

@xvcgreg
Copy link
Author

xvcgreg commented May 3, 2023

Also, I cannot use the name BC_Day01 due to some weird/unnecessary naming restrictions. Even more annoying is that it accepts the name but doesn't store it (so the user is happy that he has named the project to later discover with puzzlement that he/she might have opened some different project since one he has been working had a different name).

2305031238_shareX.mp4

@hubertp
Copy link
Collaborator

hubertp commented May 3, 2023

Tried to reproduce with latest nightly but no avail. One thing I noticed was that it takes 1-2 seconds for the finalizers to kick-in/finish, and I was tempted to just ctrl-c and kill it. Not much we could do in that case anyway.
Alternatively, we could maybe persist information about the rename action and on startup check that it really happened?

@xvcgreg
Copy link
Author

xvcgreg commented May 3, 2023

@hubertp I can confirm - with 2023.5.3 Nightly renaming folders works.

This is left still:
image

@4e6
Copy link
Contributor

4e6 commented May 3, 2023

Thanks for the confirmation. An ability to have a project name without strict validation rules will be implemented in #6356

@4e6 4e6 closed this as completed May 3, 2023
@github-project-automation github-project-automation bot moved this from ❓New to 🟢 Accepted in Issues Board May 3, 2023
@xvcgreg
Copy link
Author

xvcgreg commented May 4, 2023

@4e6 It happened again that after Enso shut down it didn't rename the folder:

image
Enso_201 - localhost-1683182927733.log
Enso_201 - console.txt

@xvcgreg xvcgreg reopened this May 4, 2023
@github-project-automation github-project-automation bot moved this from 🟢 Accepted to ❓New in Issues Board May 4, 2023
@hubertp
Copy link
Collaborator

hubertp commented May 4, 2023

One thing which I'm not seeing at the end of the logs is something along the lines of

[info] [org.enso.projectmanager.infrastructure.languageserver.LanguageServerController] Language server shut down successfully [Project(21260df1-f569-43d3-b2b2-d70944d8db8b,Warning_2,local,UserProject,xxx,Some(Edition(Some(0.0.0-dev),None,Map(),Map())),Some(xxx),Some(/home/hubert/enso/projects/Warning_2),Some(xxx))].
[info] [org.enso.projectmanager.infrastructure.languageserver.LanguageServerKiller] All language servers have been killed.
[info] [org.enso.projectmanager.infrastructure.languageserver.ShutdownHookRunner] Firing shutdown hooks for project [21260df1-f569-43d3-b2b2-d70944d8db8b].
[info] [org.enso.projectmanager.infrastructure.log.Slf4jLogging] Project [21260df1-f569-43d3-b2b2-d70944d8db8b] moved to [/home/hubert/enso/projects/Warning_3].
[info] [org.enso.projectmanager.infrastructure.languageserver.ShutdownHookRunner] All shutdown hooks fired for project [21260df1-f569-43d3-b2b2-d70944d8db8b].
[info] [akka.actor.CoordinatedShutdown] Running CoordinatedShutdown with reason [ActorSystemTerminateReason]

That would indicate renaming action being triggered. Lack of it explains why you are seeing the problem.

@xvcgreg
Copy link
Author

xvcgreg commented May 4, 2023

Yes, I have noticed that too. I have closed the app normally. And I was running it with debug flag.

@hubertp
Copy link
Collaborator

hubertp commented May 4, 2023

Oh wait. Seeing in the logs

[warn] [2023-05-04T06:49:15.991Z] [org.enso.projectmanager.infrastructure.languageserver.LanguageServerController] Received unknown message: class org.enso.projectmanager.infrastructure.languageserver.LanguageServerController$ShutDownServer$

That's a bit unexpected at this stage, right @4e6 ?

That would basically mean that the LanguageServerKiller's message is not handled appropriately and LanguageServerController is not shutdown properly; the former will eventually send a PoisonPill and shutdown hooks will not run.

@4e6 4e6 removed the s-info-needed Status: more information needed from submitter label May 4, 2023
@hubertp hubertp assigned hubertp and unassigned 4e6 May 5, 2023
@hubertp
Copy link
Collaborator

hubertp commented May 5, 2023

I will take over while Dmitry is busy

@hubertp hubertp moved this from ❓New to 🔧 Implementation in Issues Board May 5, 2023
@xvcgreg
Copy link
Author

xvcgreg commented May 8, 2023

Additional info:

After failing to comply with naming restrictions, I have used enforced styling and Enso still could not rename the project folder on exit.

image

Op9 is at

@hubertp
Copy link
Collaborator

hubertp commented May 11, 2023

@xvcgreg That verification is something else. It's done on GUI side. Probably warrants a separate ticket, right @Procrat ?

@vitvakatu
Copy link
Contributor

I don't see the GUI-side verification here. I see a response from the project manager that replies that the project name is not in upper snake case. @hubertp

@hubertp
Copy link
Collaborator

hubertp commented May 11, 2023

@vitvakatu ah yes, sorry got confused in that error.

@Procrat
Copy link
Contributor

Procrat commented May 11, 2023

The GUI-side check that was added in #6366 is done by parsing it as a Cons AST node (for sake of not duplicating validation logic), which might not be as restrictive as the check that's done on the engine side. This error is indeed from the engine (which is called "Peer" here).

@hubertp
Copy link
Collaborator

hubertp commented May 11, 2023

So I managed to create theoretically possible scenario that would cause it by adding delays within the implementation of language server:

  1. Project is renamed in the editor, all good.
  2. Enso is closed.
  3. Added a (small) timeout to LanguageServerKiller ( ) so that LanguageServerController receives the information about the disconnected client first ( )
  4. That enters the shutdown sequence in so more delays here...
  5. ... so that LanguageServerKiller eventually sends the PoisonPill . And, more importantly, that message is handled before LanguageServerController is able to receive information about the ServerTerminated in
  6. This means is never triggered, ProjectClosed message is never sent and no hooks are ever fired via

That way I managed to have a package.yaml renamed but the directory not. It may be a bit shaky, given that I had to add some weird delays in various places to simulate delayed processing/handling of messages. But it also confirms why we see it so rarely and on potentially lower spec machines. The logs seems to confirm the sequence of exchanges as well.

What we are missing, I think, is LanguageServerKiller sending ProjectClosed.

@enso-bot
Copy link

enso-bot bot commented May 12, 2023

Hubert Plociniczak reports a new STANDUP for yesterday (2023-05-11):

Progress: Spent significant amount of time trying to add artificial delays to simulate the reported bug. Investigation was hindered by the fact that it manifest itself in a standalone version due to a race-condition between shutdown triggers. And compilation loop when triggered via run ide is horribly slow. Tried to reproduce problems in #6584 but no avail. It should be finished by 2023-05-12.

Next Day: Next day I will be working on the #6515 task. Provide a fix, now that a reproducible scenario is available.

hubertp added a commit that referenced this issue May 12, 2023
This change fixes the rather elusive bug where shutdown hooks could not
be fired when shutdown was taking too long and termination was forced.

Under the circumstances described in detail in ticket #6515 there was a
small chance that we could have a shutdown race condition. Essentially
the messages received when client was disconnected and language server
forced the termination could lead to language server not sending the
public `ProjectClosed` message which triggers shutdown hook. Now we
always do.

Also made sure that multiple `ProjectClosed` messages don't lead to
firing multiple shutdown hooks, which was another possibility.
@hubertp hubertp moved this from 🔧 Implementation to 👁️ Code review in Issues Board May 15, 2023
@mergify mergify bot closed this as completed in #6665 May 15, 2023
mergify bot pushed a commit that referenced this issue May 15, 2023
This change fixes the rather elusive bug where shutdown hooks could not be fired when shutdown was taking too long and termination was forced.

Under the circumstances described in detail in ticket #6515 there was a small chance that we could have a shutdown race condition. Essentially the messages received when client was disconnected and language server forced the termination could lead to language server not sending the public `ProjectClosed` message which triggers shutdown hook. Now we always do.

Also made sure that multiple `ProjectClosed` messages don't lead to firing multiple shutdown hooks, which was another possibility.

No tests as one would have to be able to introduce different delays in various message handlers to simulate the problem.
Having ability to do such chaos testing would be nice but it is beyond the scope of this ticket.
I was able to reproduce the problem 100% with my specially crafted setup so I'm fairly confident about the change.

Closes #6515.
@github-project-automation github-project-automation bot moved this from 👁️ Code review to 🟢 Accepted in Issues Board May 15, 2023
@enso-bot
Copy link

enso-bot bot commented Jun 1, 2023

Hubert Plociniczak reports a new STANDUP for the provided date (2023-05-12):

Progress: (late standup) Finished a PR for a shutdown race condition. Investigating various caching issues (related to #6584, #6655). Started investigating #6639. It should be finished by 2023-05-12.

Next Day: Next day I will be working on the #6639 task. Continue investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
--bug Type: bug --regression Important: regression -language-server
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants