Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Memory leak 'by default' with expired clients. #20

Closed
1 of 2 tasks
46bit opened this issue Mar 25, 2017 · 2 comments
Closed
1 of 2 tasks

Memory leak 'by default' with expired clients. #20

46bit opened this issue Mar 25, 2017 · 2 comments

Comments

@46bit
Copy link
Owner

46bit commented Mar 25, 2017

If a user doesn't manually remove disconnected clients they'll continue to accumulate in Room.clients. To me this is unacceptable. I think this justifies reviving an old idea of mine, to have an optional output channel for expired clients.

Disconnected clients wouldn't be stored inside Room at all; depending on Option they would be either Dropped or emitted on a disconnected_client_tx channel.

I've been trying to figure out what features should go on Room directly. A related idea is to have a client_insertion_rx channel for asynchronously adding clients #14 . These two channels would start to really up the complexity of Room.

Design exploration

  • Room
    • Room removes from Room.clients immediately when disconnect is detected.
    • impl Stream for Room emits a disconnected Client as Err
  • As-yet-unnamed type we'll call ChannelledRoom.
    • Presumably provides the same Future-creating methods as Room. A trait could enforce this. The various future types could be parametric over that trait if the different logic could be kept to impl Sink + Stream for ChannelledRoom.
    • impl Sink + Stream for ChannelledRoom takes Err(Client)s and emits them on disconnected_client_tx. I can drop irritating Error: Clone requirements if this then returns something less precise in Err.

Tradeoffs

  • The memory leak must go. The library must be safe by default.
  • We still need to get ahold of disconnected clients, as many network services will want to reestablish connections/do logic with that.
  • This ChannelledRoom is only strictly necessary for easy combinator code. Custom futures can simply insert new clients and trivially use returned errors.
  • ChannelledRoom would make the library more complicated. It leads in a promising direction, as a generalised ReconnectingRoom might then be possible, but we're talking quite complex genericism here.
  • The value of non-custom-futures code is largely appealing/simple examples. I don't think it would be sensible very often to directly use Room.

Decisions

@46bit 46bit added this to the First relatively stable API milestone Mar 25, 2017
46bit added a commit that referenced this issue Mar 25, 2017
* `Room`'s `Stream + Sink` impls now emits `Client` as `Err` when one disconnects. #20
* `Status` type removed as it no longer made sense. Methods moved onto `Client`.
@46bit
Copy link
Owner Author

46bit commented Mar 25, 2017

Fixing the memory leak in #21 led to a major reworking, taking out things like Status that had seemed less useful as time went on.

@46bit 46bit removed this from the First relatively stable API milestone Mar 25, 2017
@46bit
Copy link
Owner Author

46bit commented Mar 25, 2017

This issue mainly relates to the memory leak. Closing in favor of doing ChannelledRoom in #14 .

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

No branches or pull requests

1 participant