Fix java.lang.RuntimeException: Document is locked by write PSI operations errors

[facboy]

Also update to google-java-format 1.17.0

[plumpy]

Oh, this is great, thank you so much for fixing this!

[cpovirk]

I got asked if I had the power to un-block this. I'm not actually sure if I do, but one question before I try:

We read the document text at line 62. If we later find that the document is blocked, we perform and postponed operations, and then we write output that was based on the document text we read earlier. Does that mean that we could undo the effects of any of the operations that we'd just postponed? If so, should we be reading the document text again (maybe retrying the whole of processFile) and operating on that? If that's difficult, would it be better to simply do nothing? I see some other formatter implementations that choose to do nothing, e.g., this code in ktfmt.

[facboy]

Yeah, that's a good spot. I've changed it to unblock and apply if the document text hasn't changed (not sure how likely that is), otherwise do nothing. Intellij's own ImportOptimizers do all their work in the returned Runnable (i.e. on the Swing thread, i assume), but I'm not sure how much more heavyweight google-java-format is compared to that, so I guess we'll just give up and wait for the next run if there is a mismatch.

[cpovirk]

Thanks. Let's see if I can in fact get this converted into a change in our internal repo, and then we'll see if anyone with permissions is up for getting it submitted.

[facboy]

i changed the import optimizer to not set document.text if

1. it hasn't actually formatted anything
2. it detects that document.text has changed since it ran its formatting

[cpovirk]

i changed the import optimizer to not set document.text if

1. it hasn't actually formatted anything
2. it detects that document.text has changed since it ran its formatting

I see this warning that getText() may be expensive.

Now, presumably it doesn't add a lot to the cost of setting the text when we're modifying the imports. And even if there is nothing to set (and so we're paying only the cost of getting the text), you make a good point that IntelliJ's own import optimizer does what looks like lots of stuff (implementation?) on the main thread, so How Bad Can It Be?

Still, do you think we could get most of the value by including only the first of your two new checks, the one that happens in whatever background thread does the work, since that one uses the result of the getText() call that we already had to make?

[facboy]

it seems to suggest using getCharsSequence() instead, i can take a look at whether it's possible instead of getText(). as far as the checks go

• the first one i see as 'less' important, it's really an optimization.
• the second is more important, i think it's the reason the formatter sometimes seems to not work - the formatter correctly formats the text and it gets set on the document, then the import optimizer comes along, having built its text in the background, and overwrites the text that the formatter has set on the document..

[cpovirk]

Oh, right, sorry, I'd somehow been thinking that the second check was a new thing and not the fix you'd previously explained.

getCharSequence looks like it makes a nice, easy optimization in any case.