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.

Sign up for GitHub

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

Jump to bottom

Exception when closing Peer #55

Closed
natebosch opened this issue Jun 9, 2020 · 2 comments · Fixed by #64
Closed

Exception when closing Peer #55

natebosch opened this issue Jun 9, 2020 · 2 comments · Fixed by #64

Comments

@natebosch
Copy link
Member

After #52 there is an exception if you call Peer.close():

  Bad state: Future already completed
  dart:async                                         _SyncCompleter.complete
  package:json_rpc_2/src/channel_manager.dart 74:22  ChannelManager.close
  package:json_rpc_2/src/client.dart 90:30           Client.close
  package:json_rpc_2/src/peer.dart 150:28            Peer.close

This is due to some combination of the ChannelManager._doneCompleter being a Completer.sync() and the fact that we also try to close the manager when the manager is reporting itself closed.

Changing the completer to be async seems to cause the returned future to never complete.

json_rpc_2/lib/src/channel_manager.dart

Line 28 in 5458a9e

final _doneCompleter = Completer.sync();

natebosch added a commit that referenced this issue Jun 9, 2020
@natebosch
Add a test showing failure to close peer
2b889f4 
Example for #55
natebosch added a commit that referenced this issue Jun 9, 2020
@natebosch
Add a test showing failure to close peer
b555b20 
Example for #55
@natebosch natebosch mentioned this issue Jun 9, 2020
Merged
@natebosch
Copy link
Member Author

cc @munificent - you did the original code review for this stuff. It's not likely but just in case you have any insight into the design and structure of the ChannelManager class that could help...

natebosch added a commit that referenced this issue Jun 9, 2020
@natebosch
Add a test showing failure to close peer (#56)
a48ca46 
Example for #55
@munificent
Copy link
Member

munificent commented Jun 26, 2020 •
edited

Uh, not only have I paged this out of my memory, I don't think I even have on cold storage disc backup. [I have no memory of this place Gandalf GIF]

@natebosch natebosch mentioned this issue Aug 9, 2020
Merged
natebosch added a commit that referenced this issue Aug 10, 2020
@natebosch
Remove ChannelManager class (#64)
413ee9d 
Fixes #55

This class was intended to manage lifecycle around a `StreamChannel`,
but in the case of a `Peer` there is a conflict having 3 instances of a
`ChannelManager` attempting to manage lifecycle constraints which
results in completing the same `Completer` instance twice. Since `Peer`
needs a different behavior than `Server` or `Client` the
`ChannelManager` abstraction isn't helpful.

- Inline the important behavior around completing `done` appropriately
  for the end of the channel `stream`, or for errors, into `Peer`,
  `Server`, and `Client`.
- Inline the important behavior around closing the channel `sink` and
  completing `done` when a `Server` or `Client` is closed. `Peer` has
  different behavior and only forwards to it's `Server` and `Client`.
  The returned future from `channel.sink.close` is ignored, since this
  future does not complete in the case where a done even can't be
  delivered to listeners.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove ChannelManager class dart-lang/json_rpc_2
2 participants
@munificent @natebosch