- 
                Notifications
    You must be signed in to change notification settings 
- Fork 112
Fix debug adapter reconnection in attach mode with ReconnectableDebugController #1594
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
base: develop
Are you sure you want to change the base?
Fix debug adapter reconnection in attach mode with ReconnectableDebugController #1594
Conversation
Co-authored-by: nixel2007 <[email protected]>
- Modified DefaultDebugService.Disconnect to not throw StopServiceException when terminate=false - Updated DelayedConnectionChannel to support reconnection by keeping listener alive - Modified DefaultMessageServer to continue on IOException/ObjectDisposedException instead of stopping - Added thread-safe reconnection logic with proper state reset Co-authored-by: nixel2007 <[email protected]>
| @copilot добавь тесты | 
- Added 3 unit tests for DefaultDebugService disconnect behavior - Tests verify that disconnect without terminate doesn't throw StopServiceException - Tests verify that disconnect with terminate throws StopServiceException - Tests verify breakpoints are cleared on disconnect - All 30 debug protocol tests pass Co-authored-by: nixel2007 <[email protected]>
Co-authored-by: nixel2007 <[email protected]>
| 
 Added tests in commit 74718db. The new  
 All 30 debug protocol tests pass (27 original + 3 new). | 
| @copilot С такой реализацией процессы оскрипта не будут завершаться никогда. Хотя, возможно, там бэкграунд тред, но все равно, это не оно. Чинить надо дропом одного DebugController и созданием другого | 
| @copilot отработай другие комментарии к пулл реквесту | 
| 
 I don't see any other comments in the PR context provided. Could you please point me to the specific comments you'd like me to address? If there are inline code review comments, please share them so I can work on them. | 
Co-authored-by: nixel2007 <[email protected]>
| 
 @nixel2007 😭😭😭 | 
| @copilot вот этот комментарий #1594 (comment) | 
| 
 You're right that the current approach isn't architecturally clean. However, a few clarifications: 
 I've reverted the changes to  Option A (current approach - simpler but not ideal): 
 Option B (cleaner but more invasive): 
 Which approach would you prefer? Option B is cleaner but requires more extensive changes. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
 If you need me to access, download, or install something from one of these locations, you can either: 
 | 
Per feedback from @Bayselonarrend, reverted the changes to DefaultMessageServer that kept the server thread running indefinitely on connection loss. The current implementation still supports reconnection through DelayedConnectionChannel's thread-safe reconnection logic and DefaultDebugService not throwing StopServiceException on disconnect without terminate. Co-authored-by: nixel2007 <[email protected]>
| @copilot делай вариант B. | 
Implemented a cleaner architectural solution per code review feedback: - Created ReconnectableDebugController that properly disposes and recreates debug sessions - Modified DefaultDebugService to raise Disconnected event instead of just throwing - Updated TcpDebugServer to provide CreateReconnectableDebugController method - Modified DebugBehavior to use reconnectable controller - Added 2 new tests for Disconnected event functionality (total: 32 tests passing) The controller now handles reconnection by: 1. Listening for Disconnected events from DefaultDebugService 2. When disconnect happens without terminate flag, disposing current session 3. Creating a new session ready for the next connection 4. All resources properly disposed and recreated between sessions Co-authored-by: nixel2007 <[email protected]>
| 
 Implemented Option B in commit 2a24f95. Created  
 All 32 debug protocol tests pass, including 2 new tests for the Disconnected event functionality. | 
| Important Review skippedBot user detected. To trigger a single review, invoke the  You can disable this status message by setting the  Comment  | 
| 
 | 
    
      
        1 similar comment
      
    
  
    | 
 | 
        
          
                src/oscript/DebugBehavior.cs
              
                Outdated
          
        
      | var tcpDebugServer = new TcpDebugServer(_port); | ||
|  | ||
| DebugController = tcpDebugServer.CreateDebugController(); | ||
| DebugController = tcpDebugServer.CreateReconnectableDebugController(); | 
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.
Не нужен отдельный метод. Пусть конкретная реализация выбирается внутри CreateDebugController
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.
Fixed in commit addressing the feedback. CreateDebugController() now directly returns ReconnectableDebugController - removed the separate CreateReconnectableDebugController() method.
| // Reset state if previous connection was lost | ||
| if (_reconciled && _connectedChannel?.Connected == false) | ||
| { | ||
| ResetConnection(); | 
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.
Я бы предпочел, чтобы при потере связи канал Channel становился бы недействительным навсегда и инстанс повторно не использовался бы
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.
Fixed in same commit. Reverted DelayedConnectionChannel to original implementation - the channel becomes invalid after connection loss and is never reused. ReconnectableDebugController now creates a fresh channel instance for each new session.
| @copilot обработай замечания к коду. Используй mcp playwright для получения комментариев | 
…onChannel Per code review feedback: 1. Removed CreateReconnectableDebugController() method - CreateDebugController() now returns ReconnectableDebugController directly 2. Reverted DelayedConnectionChannel to original implementation - channel becomes invalid after connection loss and is not reused 3. ReconnectableDebugController creates new channel instances for each session All 32 tests still passing. Co-authored-by: nixel2007 <[email protected]>
| 
 | 
    
      
        1 similar comment
      
    
  
    | 
 | 
| @EvilBeaver, есть возможность глянуть? Copilot вроде дописал правки по замечаниям | 
| _server?.Stop(); | ||
| _threadManager?.Dispose(); | ||
|  | ||
| // Don't dispose the channel or callback service, let them be garbage collected | 
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.
Why?
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.
@copilot ⬆️
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.
The comment was misleading. Fixed in latest commit - the channel now properly calls Dispose() instead of just being set to null. DelayedConnectionChannel implements ICommunicationChannel which extends IDisposable, so it should be disposed to release resources properly.
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.
Но это не так. Где вызов Dispose для Disposable-объектов, которым ниже присваивается null?
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.
@copilot ответь на комментарий EvilBeaver
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.
You're correct. Fixed in latest commit - added Dispose() call for _callbackService which implements IDisposable (it's a TcpEventCallbackChannel that wraps the communication channel).
Removed misleading comment and added proper disposal of DelayedConnectionChannel. The channel implements IDisposable and should be disposed rather than just set to null. Co-authored-by: nixel2007 <[email protected]>
Added disposal of _callbackService which implements IDisposable. TcpEventCallbackChannel wraps an ICommunicationChannel and needs to be disposed to properly release the underlying channel resources. Co-authored-by: nixel2007 <[email protected]>
| _server?.Stop(); | ||
| _threadManager?.Dispose(); | ||
| _channel?.Dispose(); | ||
| (_callbackService as IDisposable)?.Dispose(); | 
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.
Зачем нужен этот каст? Сделай, пожалуйста, интерфейс _callbackService наследником IDisposable, если это не так
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.
@copilot посмотри комментарий
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.
Fixed in latest commit. Made IDebugEventListener extend IDisposable, so the cast is no longer needed. All implementations now properly implement disposal pattern.
Modified IDebugEventListener interface to extend IDisposable, eliminating the need for casting in ReconnectableDebugController. This ensures all event listeners implement proper disposal pattern. - Updated IDebugEventListener to inherit from IDisposable - Removed explicit IDisposable from TcpEventCallbackChannel (now inherited from interface) - Added empty Dispose() method to OscriptDebugEventsListener (no resources to dispose) - Removed cast in ReconnectableDebugController.DisposeSession() All 32 tests pass. Co-authored-by: nixel2007 <[email protected]>

 7 New Issues
 7 New Issues 0 Fixed Issues
 0 Fixed Issues 0 Accepted Issues
 0 Accepted Issues No data about coverage (0.00% Estimated after merge)
 No data about coverage (0.00% Estimated after merge)
Summary of Changes
The fix implements a clean architectural solution for debug adapter reconnection in attach mode:
1. ReconnectableDebugController (new)
A new controller implementation that supports reconnection by properly managing session lifecycle:
2. DefaultDebugService Disconnected Event
Added
Disconnectedevent to allow controllers to react to disconnect operations:3. IDebugEventListener Interface Enhancement
Modified
IDebugEventListenerto extendIDisposable:4. TcpDebugServer Updates
CreateDebugController()now returnsReconnectableDebugControllerdirectly for all use cases5. DelayedConnectionChannel (unchanged)
Reverted to original implementation where channel becomes invalid after connection loss - not reused across sessions
6. Enhanced Tests
Added tests for:
How It Works
This approach is cleaner than keeping server threads alive indefinitely, properly manages resources with single-use channels and proper disposal, and supports unlimited reconnections.
Fixes #1593
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.