Stream Inspector: Close inspector when connector/processor is deleted

[hariso]

Description

Fixes #743

Quick checks:

• I have followed the Code Guidelines.
• There is no other pull request for the same update/change.
• I have written unit tests.
• I have made sure that the PR is of reasonable size and can be easily reviewed.

[lovromazgon]

If we are strict, we should be locking i.lock before we loop through i.sessions, but then s.close() also tries to lock it and we have a deadlock 😕 For correctness' sake we might want to lock first, then copy i.sessions into a separate map/slice, then unlock and then loop through the copied map/slice to close the sessions. It still doesn't guarantee that a new session won't be created while this is happening.

I know, it's an edge case, but I'm trying to say that something feels off regarding this design 😕

[hariso]

I wasn't too happy with these locks from the beginning, and even less now. The thing is that the lock handles concurrent access to the sessions' map, but what we actually need to do is to make sure that the inspector will refuse to create a new session ever again after it's been closed.

IMHO, the edge case is rare enough that working more on this design doesn't return much, especially after we move inspector.Close() after connector/processor deletion. At that point, starting a new session won't be possible at all (the API will complain that the processor/connector doesn't exist).

[lovromazgon]

I see, if we delete the entity first and then close the inspector, there can't be any more requests to create new sessions. Still, it doesn't feel right to have a component that can behave unexpectedly if it's used the wrong way and we rely on using it correctly elsewhere 😕 Don't worry, I won't block the PR because I feel bad 😅 I think we can do better though.

I thought about the locking and especially the optimization TODO in Send. I gave it a go and spiked out a slightly different approach to managing sessions in the inspector using a channel (see 2796796). The main difference would be that we need an upper bound on how many sessions we can create (I think it would be smart to have that anyway). We still can't get away without a lock, it's needed because we want to close the session channel in a different goroutine than the one that's writing to that channel, without a lock we risk a panic. The nice thing about this approach is that we don't need to lock anything to check for open sessions (using a map we don't get around that), so if there are no open sessions the performance is 2x as fast (and that's IMO the case we need to optimize for).

There are some TODOs in that spike that would need to be addressed if we wanted to go down that route. Let me know what you think and if we should pursue this in a separate PR.

(BTW: I'll keep this comment unresolved so we can discuss it, but you can merge the PR anyway)

[hariso]

Still, it doesn't feel right to have a component that can behave unexpectedly if it's used the wrong way and we rely on using it correctly elsewhere confused

I definitely agree with that and my point was that it reduces the changes of something which is an edge case already.

I gave it a go and spiked out a slightly different approach to managing sessions in the inspector using a channel (see 2796796).

Really nice! 👍 It solves some of the annoyances we currently have. I was initially thinking about something like a slice or a channel to store the sessions, but didn't go with it because I wanted the sessions to be removed and not stay around after they are not needed anymore.

The nice thing about this approach is that we don't need to lock anything to check for open sessions

When you say "to check for open sessions" which part do you refer to?

[lovromazgon]

I'm talking about this part where we return early if there are no open sessions. If we did the same thing with a map we would need to acquire a lock first.