bsl-language-server icon indicating copy to clipboard operation
bsl-language-server copied to clipboard
verified publisher icon 1c-syntax

Fix NoSuchElementException in MissingCommonModuleMethodDiagnostic

Open Copilot opened this issue 3 months ago • 7 comments

Описание

NoSuchElementException возникала в getMethodNameByLocation() при вызове orElseThrow() когда findTerminalNodeContainsPosition() возвращал пустой Optional (при рассинхронизации индекса ссылок и AST).

Изменения:

  • getMethodNameByLocation() теперь возвращает Optional<String> вместо выброса исключения
  • getReferenceToMethodCall() использует flatMap для корректной обработки пустого результата
// Было:
private static String getMethodNameByLocation(ParserRuleContext node, Range range) {
  return Trees.findTerminalNodeContainsPosition(node, range.getStart())
    .map(ParseTree::getText)
    .orElseThrow();  // ← NoSuchElementException
}

// Стало:
private static Optional<String> getMethodNameByLocation(ParserRuleContext node, Range range) {
  return Trees.findTerminalNodeContainsPosition(node, range.getStart())
    .map(ParseTree::getText);
}

Связанные задачи

Closes https://1c-syntax.sentry.io/issues/7081340854/

Чеклист

Общие

  • [x] Ветка PR обновлена из develop
  • [x] Отладочные, закомментированные и прочие, не имеющие смысла участки кода удалены
  • [x] Изменения покрыты тестами
  • [x] Обязательные действия перед коммитом выполнены (запускал команду gradlew precommit)

Для диагностик

  • [x] Описание диагностики заполнено для обоих языков (присутствуют файлы для обоих языков, для русского заполнено все подробно, перевод на английский можно опустить)

Дополнительно

Все 963 теста проходят успешно. CodeQL проверка не выявила проблем безопасности.

Original prompt

This section details on the original issue you should resolve

<issue_title>NoSuchElementException: No value present</issue_title> <issue_description>Sentry Issue: BSL-LANGUAGE-SERVER-KG

NoSuchElementException: No value present
    at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.getMethodNameByLocation(MissingCommonModuleMethodDiagnostic.java:60)
    at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.getReferenceToMethodCall(MissingCommonModuleMethodDiagnostic.java:93)
    at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.check(MissingCommonModuleMethodDiagnostic.java:74)
    at com.github._1c_syntax.bsl.languageserver.diagnostics.AbstractDiagnostic.getDiagnostics_aroundBody0(AbstractDiagnostic.java:44)
    at com.github._1c_syntax.bsl.languageserver.diagnostics.AbstractDiagnostic$AjcClosure1.run(AbstractDiagnostic.java:1)
...
(28 additional frame(s) were not displayed)
```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@nixel2007</author><body>

java.util.NoSuchElementException: No value present at java.util.Optional.orElseThrow(Optional.java:377) at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.getMethodNameByLocation(MissingCommonModuleMethodDiagnostic.java:60) at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.getReferenceToMethodCall(MissingCommonModuleMethodDiagnostic.java:93) at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:214) at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:197) at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:197) at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:197) at java.util.concurrent.ConcurrentHashMap$KeySpliterator.forEachRemaining(ConcurrentHashMap.java:3589) at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:570) at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560) at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:153) at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:176) at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265) at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:632) at com.github._1c_syntax.bsl.languageserver.diagnostics.MissingCommonModuleMethodDiagnostic.check(MissingCommonModuleMethodDiagnostic.java:74) at com.github._1c_syntax.bsl.languageserver.diagnostics.AbstractDiagnostic.getDiagnostics_aroundBody0(AbstractDiagnostic.java:44) at com.github._1c_syntax.bsl.languageserver.diagnostics.AbstractDiagnostic$AjcClosure1.run(AbstractDiagnostic.java:1) at org.aspectj.runtime.reflect.JoinPointImpl.proceed(JoinPointImpl.java:164) at com.github._1c_syntax.bsl.languageserver.aop.MeasuresAspect.measureBSLDiagnostic(MeasuresAspect.java:63) at com.github._1c_syntax.bsl.languageserver.diagnostics.AbstractDiagnostic.getDiagnostics(AbstractDiagnostic.java:42) at com.github._1c_syntax.bsl.languageserver.context.computer.DiagnosticComputer.lambda$internalCompute$1(DiagnosticComputer.java:82) at java.util.stream.ReferencePipeline$7$1FlatMap.accept(ReferencePipeline.java:288) at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1716) at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:570) at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560) at java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2191) at java.util.stream.Nodes$CollectorTask.doLeaf(Nodes.java:2157) at java.util.stream.AbstractTask.compute(AbstractTask.java:327) at java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:759) at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:511) at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1450) at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:2019) at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:187)

</comments>
  • Fixes 1c-syntax/bsl-language-server#3635

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot avatar Dec 03 '25 08:12 Copilot

[!IMPORTANT]

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Dec 03 '25 08:12 coderabbitai[bot]

@copilot в какой момент может произойти рассинхронизация? Проведи детальный анализ многопоточного взаимодействия с учётом блокировок

nixel2007 avatar Dec 03 '25 08:12 nixel2007

@copilot в какой момент может произойти рассинхронизация? Проведи детальный анализ многопоточного взаимодейств...

Анализ многопоточного взаимодействия:

Рассинхронизация между LocationRepository и AST возможна в следующих сценариях:

1. Race condition при обновлении документа (наиболее вероятный сценарий)

Thread A (ReferenceIndexFiller.handleEvent):     Thread B (DiagnosticComputer.internalCompute):
──────────────────────────────────────────────  ──────────────────────────────────────────────
index.clearReferences(uri)                      locationRepository.getSymbolOccurrencesByLocationUri(uri)
                                                   ↓ получает SymbolOccurrence из "старого" индекса
↓ начинает строить новый индекс
new MethodSymbolReferenceIndexFinder().visit()    ↓ MissingCommonModuleMethodDiagnostic.check()
                                                     ↓ getMethodNameByLocation(documentContext.getAst(), range)
↓ добавляет новые SymbolOccurrence                      ↓ AST уже обновлён, а Range из старого индекса!
                                                           → findTerminalNodeContainsPosition возвращает empty

2. Архитектурная особенность:

  • LocationRepository.locations — ConcurrentHashMap<URI, Set<SymbolOccurrence>>
  • Set<SymbolOccurrence> — ConcurrentHashMap.newKeySet()
  • Операция getSymbolOccurrencesByLocationUri(uri) возвращает Stream над мутируемой коллекцией
  • Между итерацией по стриму в check() и чтением AST нет синхронизации

3. Блокировки в DocumentContext:

  • computeLock защищает getAst(), rebuild(), clearSecondaryData()
  • diagnosticsLock защищает computeDiagnostics()
  • Но нет блокировки между ReferenceIndexFiller и DiagnosticComputer

Вывод: Исправление корректно — graceful handling через Optional является правильным подходом для данного race condition, поскольку:

  1. Добавление синхронизации между индексом и диагностиками сильно усложнит архитектуру
  2. Ситуация редкая и не критичная — просто пропускаем конкретное вхождение символа

Copilot avatar Dec 03 '25 08:12 Copilot

@copilot ситуация рассинхронизации на ast частая. Попадает ли reference index filler с эвента под computeLock, стоящий вокруг rebuild? Ну и DiagnosticsComputer закрыт diagnostic compute lock'ом

nixel2007 avatar Dec 03 '25 09:12 nixel2007

@copilot ситуация рассинхронизации на ast частая. Попадает ли reference index filler с эвента под computeLock, стоящий вокруг rebuild? ...

Проанализировал детальнее:

ReferenceIndexFiller и computeLock:

  • Event публикуется через @AfterReturning — т.е. ПОСЛЕ завершения rebuild(), когда lock уже освобождён
  • ReferenceIndexFiller.fill() вызывает documentContext.getAst(), который защищён @Locked("computeLock")
  • Таким образом, ReferenceIndexFiller получает консистентный AST

Проблема в другом:

getReferenceToMethodCall():
  ↓ symbolOccurrence.location().getRange()  // Range из LocationRepository (не защищён никаким lock'ом)
  ↓ documentContext.getAst()                // AST защищён computeLock

LocationRepository — отдельный ConcurrentHashMap, не связанный с computeLock/diagnosticsLock. Range берётся из SymbolOccurrence, который был создан в момент предыдущего fill() с предыдущим AST.

Сценарий:

  1. Document A rebuild → event → ReferenceIndexFiller.fill(A) → создаёт SymbolOccurrence с Range_v1
  2. Document A rebuild (новая версия) → AST обновляется на AST_v2
  3. Параллельно DiagnosticComputer запрашивает диагностики для A
  4. check() читает из LocationRepository старый SymbolOccurrence с Range_v1
  5. getAst() возвращает AST_v2
  6. Range_v1 не находится в AST_v2 → empty Optional

diagnosticsLock защищает только computeDiagnostics(), но не синхронизирует обновление LocationRepository с вычислением диагностик.

Copilot avatar Dec 03 '25 09:12 Copilot

@copilot тот момент, когда начнёт выполняться второй ребилд, будет висеть computeLock, diagnostics не смогут запросить старый ast

nixel2007 avatar Dec 03 '25 11:12 nixel2007

@copilot тот момент, когда начнёт выполняться второй ребилд, будет висеть computeLock, diagnostics не смогут запросить стары...

Согласен — computeLock + diagnosticsLock защищают AST: пока идёт rebuild(), диагностики ждут, и наоборот. Для одного документа рассинхронизации AST быть не должно.

Однако ошибка реально происходит (Sentry). Возможные сценарии:

  1. Битый/нестандартный код: Trees.findTerminalNodeContainsPosition() не находит ноду по позиции, если Range указывает на пустую область (начало > конца, или после EOF)

  2. Проблема в парсере/токенизаторе: если AST невалидный после парсинга (ошибки синтаксиса), позиция может не найтись

  3. Edge case в самой findTerminalNodeContainsPosition(): метод возвращает Optional.empty() при tree.getTokens().isEmpty() или если позиция вне диапазона start/stop токенов

Независимо от причины, orElseThrow() не должен падать в runtime — graceful handling через Optional защищает от любого из этих сценариев. Можно добавить логирование, чтобы понять первопричину в проде.

Copilot avatar Dec 03 '25 11:12 Copilot