0

I recently have had to answer some coding exercises for a job interview but the following one has me stumped. I think that the code is supposed to be part of a web service used by a mobile app.

Q: What are potential problems with the code below? How would you fix it?

public class Service
{  
  public event EventHandler<EventArgs> ItemCreated;

  private Dictionary<Guid, Listener> _listeners = new Dictionary<Guid, Listener>();

  public Guid Login()
  {
     var listener = new Listener(this);
     var token = Guid.NewGuid();
     _listeners[token] = listener;
     return token;
  }

  public void Logout(Guid token)
  {
     _listeners.Remove(token);
  }

}

public class Listener
{
  public Listener(Service service)
  {
   service.ItemCreated += OnItemCreated;
  }

  private void OnItemCreated(object sender, EventArgs e)
  {      }
}

I have put the code into a project and it runs without any issues.

My best shot so far is that there doesn't seem to be anywhere that the Event is triggered, but because the code doesn't have any further explanation that would allow for a fix I am doubting if I am correct on that.

UPDATE

Thanks for the answers. I have updated with my suggested fix.

public void Logout(Guid token)
{
   _listeners[token].Unsubscribe(this);
   _listeners.Remove(token);
}

public class Listener
{
  ...

  public void Unsubscribe(Service service)
  {
      service.ItemCreated -= OnItemCreated;
  }
}
Xtravia9
  • 63
  • 1
  • 8

1 Answers1

2

The main issue with this code which interviewer was addressing, I think, is that on Logout listener is removed from _listeners dictionary but is not unsubscribed from service.ItemCreated event which will lead to memory leak, cause GC will not be able to collect it (cause event will hold the reference to listener). Also, as @Klaus Gütter stated in comment to my answer, it can possibly lead to security/business logic problems in the application, depended on actual context.

Guru Stron
  • 102,774
  • 10
  • 95
  • 132
  • 3
    Not only a matter of memory leak but maybe also a security breach if the client can still talk to the server after logging out. – Klaus Gütter Oct 05 '20 at 15:53
  • 2
    I'd also be asking questions about concurrency and thread safety re modifications to the dictionary – Marc Gravell Oct 05 '20 at 16:02
  • 1
    @MarcGravell for sure that is another concern. But I don't see like no signs that this code was supposed to work in multithreaded environment without external synchronization so went for more obvious interview concern in regards of events =) – Guru Stron Oct 05 '20 at 16:05
  • Thanks for that. I have updated with a suggested fix. – Xtravia9 Oct 05 '20 at 16:11
  • @Xtravia9 was glad to help! Small note though. Despite fixing the supposed issue in this example in actual production code I would recommend to refactor this - you are tightly coupling `Service` and `Listener` (also can be said about original snippet too) which can reduce maintainability and testability of the code. – Guru Stron Oct 05 '20 at 17:47
  • 1
    @GuruStron that's why I said "asking questions" and not "making changes"; it matters, and I'd want an answer to that – Marc Gravell Oct 05 '20 at 21:40