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

Fix Connection is being forcefully terminated error in GUI #3829

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andrei-toterman
Copy link
Contributor

we have bidirectional streaming rpcs, but we usually send only one response back on that stream for most operations. therefore, the gui would only read data from the stream until it got the first reply. but this introduced errors in which the stream was disposed too early. now we read everything from the stream, so that issue shouldn't happen.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.95%. Comparing base (7074116) to head (7f7d7fd).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3829   +/-   ##
=======================================
  Coverage   88.94%   88.95%           
=======================================
  Files         256      256           
  Lines       14584    14584           
=======================================
+ Hits        12972    12973    +1     
+ Misses       1612     1611    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@andrei-toterman andrei-toterman force-pushed the fix-http2-connection-disconnected branch from ca75b7a to a2ecad8 Compare December 10, 2024 19:49
@andrei-toterman andrei-toterman force-pushed the fix-http2-connection-disconnected branch 2 times, most recently from 8fab6a6 to 0e909a0 Compare December 11, 2024 11:33
Base automatically changed from terminal-zoom to main December 12, 2024 19:51
@Sploder12
Copy link
Contributor

@andrei-toterman, could you share repro steps for the issue this solves?

@andrei-toterman
Copy link
Contributor Author

@Sploder12 The problem is that it doesn't happen all the time. But you can try having two stopped instances, go to the instance list page, start one instance and then immediately try starting the other one. That was one way I managed to reproduce it a few times. The other way I could reproduce it all the time is with the new 'Open shell' button functionality, which would start the instance if its stopped. But that PR has this branch as a base, precisely because the issue would occur all the time. So if you want a good way to test it, you could rebase that branch on main, see if the issue occurs, than rebase it back on this branch and see the issue not occur anymore. Sorry for not having better steps, it's kind of an elusive issue 😅

@Sploder12
Copy link
Contributor

@Sploder12 The problem is that it doesn't happen all the time. But you can try having two stopped instances, go to the instance list page, start one instance and then immediately try starting the other one. That was one way I managed to reproduce it a few times. The other way I could reproduce it all the time is with the new 'Open shell' button functionality, which would start the instance if its stopped. But that PR has this branch as a base, precisely because the issue would occur all the time. So if you want a good way to test it, you could rebase that branch on main, see if the issue occurs, than rebase it back on this branch and see the issue not occur anymore. Sorry for not having better steps, it's kind of an elusive issue 😅

Ah, I see, thanks!

Copy link
Contributor

@Sploder12 Sploder12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a small DRY comment

src/client/gui/lib/grpc_client.dart Outdated Show resolved Hide resolved
@andrei-toterman andrei-toterman force-pushed the fix-http2-connection-disconnected branch from 82e8fb4 to 2c63e3d Compare December 16, 2024 22:27
we have bidirectional streaming rpcs, but we usually send only one
response back on that stream for most operations. therefore, the gui
would only read data from the stream until it got the first reply. but
this introduced errors in which the stream was disposed too early. now
we read everything from the stream, so that issue shouldn't happen.
@andrei-toterman andrei-toterman force-pushed the fix-http2-connection-disconnected branch from 2c63e3d to acf4d8f Compare December 16, 2024 22:28
Sploder12
Sploder12 previously approved these changes Dec 16, 2024
Copy link
Contributor

@Sploder12 Sploder12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Nice job!

@andrei-toterman
Copy link
Contributor Author

@Sploder12 oops, the behavior after my refactor was not consistent with the previous behavior. Before, we were not logging all of the requests and replies for the info rpc, because we poll it frequently, so it would've been too much logging. But with the refactor, now there were constant logs for all the sent info requests. Now it should be fixed.

Copy link
Contributor

@Sploder12 Sploder12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I missed that too. I paid extra attention to the logging this time and it looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants