-
Notifications
You must be signed in to change notification settings - Fork 105
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
Feature/websocket Поддержка работы через websocket #2567
Conversation
ну нихрена себе! спасибо, заценим. |
src/main/java/com/github/_1c_syntax/bsl/languageserver/cli/WebsocketStartCommand.java
Outdated
Show resolved
Hide resolved
footer = "@|green Copyright(c) 2018-2020|@") | ||
@Component | ||
@RequiredArgsConstructor | ||
public class WebsocketStartCommand implements Callable<Integer> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salexdv в рамках реализации LanguageServerStartCommand общение через веб-сокет не реализовать? Это ведь только транспорт, верно?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Наверно, это можно назвать транспортом. Делал по аналогии с LanguageServerStartCommand
src/main/java/com/github/_1c_syntax/bsl/languageserver/websocket/WebSocketRunner.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/websocket/WebSocketRunner.java
Outdated
Show resolved
Hide resolved
private final LanguageServerConfiguration configuration; | ||
private final WebSocketRunner launcher; | ||
|
||
public Integer call() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@salexdv нужны тесты
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тесты добавил. Проверяется запуск сервера и возможность подключения к нему. Таких проверок будет достаточно?
Прилагаю "клиенты" для тестирования test_client.zip С простым текстом модуля |
@salexdv а есть стак-трейс и trace.log? |
@qtLex тут трейс лог по переменным в индексе. глянешь? |
@nixel2007 Гляну. |
Влил, спасибо! |
|
||
@Override | ||
protected void configure(Builder<LanguageClient> builder) { | ||
builder.setLocalService(BSLLSBinding.getApplicationContext().getBean(BSLLanguageServer.class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Использование BSLLSBinding чем-то обосновано? Обычное внедрение зависимостей не работало?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Хм. Можем попробовать еще раз? на класс повесить аннотацию @RequiredArgsConstructor
, и объявить поле private final BSLLanguageServer languageServer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
@Override | ||
protected void connect(Collection<Object> localServices, LanguageClient remoteProxy) { | ||
LanguageClientHolder clientHolder = BSLLSBinding.getApplicationContext().getBean(LanguageClientHolder.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Более правильно это делать через внедрение List
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Не очень понимаю, о чём речь. Можешь подробнее написать или подсказать, куда посмотреть, что почитать?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
добавить поле private final List<LanguageClientAware> languageClientAwares;
и обойти их в цикле, вызвав у каждого connect
. Как пример - LanguageServerStartCommand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Попробовал так:
@RequiredArgsConstructor
public class BSLLSWebSocketEndpoint extends WebSocketEndpoint<LanguageClient> {
private final List<LanguageClientAware> languageClientAwares;
@Override
protected void connect(Collection<Object> localServices, LanguageClient remoteProxy) {
languageClientAwares.forEach(languageClientAware -> languageClientAware.connect(remoteProxy));
}
}
Поведение у клиента становится таким же, как выше описывал для @RequiredArgsConstructor
и private final BSLLanguageServer languageServer
src/main/java/com/github/_1c_syntax/bsl/languageserver/websocket/WebSocketRunner.java
Outdated
Show resolved
Hide resolved
|
||
String contextPath = "/"; | ||
var server = new Server(hostname, port, contextPath, null, BSLLSWebSocketServerConfigProvider.class); | ||
Runtime.getRuntime().addShutdownHook(new Thread(server::stop, "bsl-websocket-server-shutdown-hook")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
может быть здесь подойдет использование родного спрингового @PreDestroy вместо shudown hook целиком JVM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addShutdownHook убрал
} catch (Exception e) { | ||
LOGGER.error("Cannot start websocket server.", e); | ||
} finally { | ||
server.stop(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не совсем понимаю, зачем stop и тут и в preshutdown hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Этот блок тоже убрал
|
||
try { | ||
server.start(); | ||
Thread.currentThread().join(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
поясни, плз, зачем здесь join?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Скорее всего напишу полную фигню т.к. опыта разработки на java мне явно не хватает. Тут приостанавливаем выполнение текущего потока, чтобы иметь возможность обработать InterruptedException
и там остановить websocket-сервер. Долго сидел с этим, но другого способа нормально работать, в том числе тесты, не нашел. Если не обработать прерывание потока, тогда ws-сервер при окончании текста продолжает висеть на порту и следующий тест выдает Address already in use: bind
src/main/java/com/github/_1c_syntax/bsl/languageserver/cli/WebsocketStartCommand.java
Outdated
Show resolved
Hide resolved
@Slf4j | ||
@Component | ||
@RequiredArgsConstructor | ||
public class WebSocketRunner { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю немного привести API в соответствие с LSP. Переименовать класс в WebSocketLauncher, а основной метод в startListening.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может быть вообще сделать его implements Launcher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тогда и внедрять его можно будет по какому-нибудь условию прямо в команду запуска в режиме LSP. Покурю этот момент
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Переименовал класс в WebSocketLauncher, а основной метод в startListening
Нужно врубать отладочные логи и глядеть... Тестовый клиент ты скидывал, да?
Op wo 20 jul. 2022 18:49 schreef Alexander Shkuraev <
***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In
src/main/java/com/github/_1c_syntax/bsl/languageserver/websocket/BSLLSWebSocketEndpoint.java
<#2567 (comment)>
:
> +import com.github._1c_syntax.bsl.languageserver.BSLLSBinding;
+import com.github._1c_syntax.bsl.languageserver.BSLLanguageServer;
+import com.github._1c_syntax.bsl.languageserver.LanguageClientHolder;
+import org.eclipse.lsp4j.jsonrpc.Launcher.Builder;
+import org.eclipse.lsp4j.services.LanguageClient;
+import org.eclipse.lsp4j.websocket.WebSocketEndpoint;
+import org.springframework.stereotype.Component;
+
+import java.util.Collection;
+
***@***.***
+public class BSLLSWebSocketEndpoint extends WebSocketEndpoint<LanguageClient> {
+
+ @OverRide
+ protected void configure(Builder<LanguageClient> builder) {
+ builder.setLocalService(BSLLSBinding.getApplicationContext().getBean(BSLLanguageServer.class));
Пробовал так сделать. Сервер стартует, но клиент не соединяется с ним.
Вернее соединение устанавливается, клиент вызывает метод initialize и
отваливается. Затем заново попытка создать соединение и так по кругу.
[image: chrome_4SK2fxQiUx]
<https://user-images.githubusercontent.com/6356534/180026172-a2894b60-cb42-4f1e-9c00-036ad86c14f8.gif>
[image: BSL_Unsupported_0]
<https://user-images.githubusercontent.com/6356534/180026468-1f8d6dc9-9234-4768-b84a-aaf9809f7efe.png>
—
Reply to this email directly, view it on GitHub
<#2567 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIUSKEG44GCLD7TQDJJ3SDVVANXFANCNFSM5PHP5A3A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Да, выше тут выкладывал |
Была у меня такая проблема:
Проблему решил, но не уверен, что сделал это правильно. Решение такое: // Класс WebsocketCommand
LanguageServerConfiguration configuration = BSLLSBinding.getLanguageServerConfiguration();
configuration.update(configurationFile); Изначально была аннотация |
Точно так же, через private final поле. Это класс-синглтон, все классы, которые запрашивают себе конфигурацию, получат один и тот же её инстанс. Соответственно если ты в команде сделаешь configuration.update, то она обновится во всех классах сразу. Вообще единственное назначение BSLLSBinding - инициализация bsl ls вне спринг контекста, как например в сонар-плагине. Здесь же запуск из cli, поэтому все должно работать штатно, через внедрение зависимостей |
Это понятно. Изначально по этому пути и пошёл, но не работает. |
ec83c60
to
fa06596
Compare
@salexdv тэкс, я таки осилил перевод веб-сокетов на ядро спринга. просьба протестировать. @asosnoviy @otymko присоединяйтесь к веселью. P.S. Из того что обнаружил - в "тест-клиенте" довольно старая версия монако/лэнгклиента и нет свежих фич. плюс клиент не шлет workspace в initialize, из-за этого не подхватываются сорцы из конфиг файла. но это вроде бы все решаемо. |
fa06596
to
d8d896b
Compare
@@ -1,4 +1,4 @@ | |||
spring.main.web-application-type=none | |||
server.port=8025 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Это бы сразу указать в доке.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
В джавадоке указано, в маркдаун еще перенести надо
src/main/java/com/github/_1c_syntax/bsl/languageserver/BSLTextDocumentService.java
Show resolved
Hide resolved
src/main/java/com/github/_1c_syntax/bsl/languageserver/cli/WebsocketCommand.java
Show resolved
Hide resolved
|
||
@Component | ||
@RequiredArgsConstructor | ||
public class BSLLSWebSocketEndpoint extends WebSocketEndpoint<LanguageClient> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Жабадок спасет мир=)
|
||
@Configuration | ||
@EnableWebSocket | ||
public class WebSocketConfiguration { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Жабадок спасет мир
src/main/java/com/github/_1c_syntax/bsl/languageserver/websocket/WebSocketConfiguration.java
Outdated
Show resolved
Hide resolved
d8d896b
to
bf79d27
Compare
bf79d27
to
d93809b
Compare
Используется версия monaco 0.20.0. Более новые версии отказываются запускаться внутри 1С. Для использования в вебе вполне можно использовать последнюю. |
@salexdv поменял эндпоинт с /bsl-language-server на /lsp. Порт по умолчанию 8025. И порт и путь можно переопределить из командой строки. |
Kudos, SonarCloud Quality Gate passed! |
Описание
Поддержка общения в BSL Language Server через websocket
Связанные задачи
#1427
Чеклист
Общие
gradlew precommit
)Дополнительно
Хотелось бы иметь возможность общаться BSL Language Server через websocket. Например, это нужно для редактора на основе monaco editor, работающего внутри 1С через webkit
Сложности, недоработки, вопросы
С разработкой на Java был мало знаком, до того как решил добавить указанную функциональность, отсюда есть несколько сложностей, которые мешают полноценной работе сервера через вебсокет.
@RequiredArgsConstructor
и поле классаprivate final LanguageServerConfiguration
. При обработке команды в классеWebsocketStartCommand
никаких проблем с чтением и обновлением конфигурации из указанного файла не возникает. Однако, не ясно, как конфигурацию можно передать вBSLLSWebSocketEndpoint
для дальнейшей передачи вBSLLanguageServer
. Можете подсказать, как решить эту проблему?