Fix handling of AsNs streams

[rvolosatovs]

Summary

Fixes handling of As-Ns streams

Changes

• Close AS streams when NS closed
• Close AS streams in a nicer way

Notes for Reviewers

Initially was part of #858

[rvolosatovs]

Blocked by #858

[coveralls]

Coverage decreased (-0.02%) to 73.086% when pulling 14938da4d01b5bd9b5fce8abab2dec9113535ef6 on rvolosatovs:fix/as-ns-close into 14be53a on TheThingsNetwork:master.

[johanstokking]

Can't we simply use builtin context cancellation? That supports waiting as well

[rvolosatovs]

@johanstokking can you clarify what you mean exactly and how it would work?
Do you mean that LinkApplication would create a new context wrapped into context.WithCancel and Close() would call the context.CancelFunc returned?
How would LinkApplication determine if io.EOF should be returned or ctx.Error()?
In order for this approach to work, LinkApplication would need to maintain 2 completely separate contexts and Close() would need to be responsible for deleting the stream from the map. I don't see benefits of such approach, whereas the proposed solution is simpler and more efficient.

[johanstokking]

Maybe I don't get it but this implementation is not obvious to me.

How would LinkApplication determine if io.EOF should be returned or ctx.Error()?

If you want to differentiate between the client closing the context (context.Canceled) and another link replacing the current (io.EOF), use pkg/errorcontext.

In order for this approach to work, LinkApplication would need to maintain 2 completely separate contexts and Close() would need to be responsible for deleting the stream from the map. I don't see benefits of such approach, whereas the proposed solution is simpler and more efficient.

Why two separate contexts? It's the same thing right? You derive a cancelable context from the link context, and that context can be canceled (potentially with a custom error) by a new link.

[johanstokking]

So Close() doesn't close but waits for a close

[rvolosatovs]

It triggers the "closing sequence" in LinkApplication or returns immediately if it is already closed

[johanstokking]

You may have ctx.Done() so you don't guarantee writing to closeCh

[rvolosatovs]

Yes, that's expected.
The write only succeeds when Close() is called at least once

[rvolosatovs]

Is there a problem with the proposed implementation?
It's really very simple - whenever Close() is called(possibly multiple times and/or concurrently) LinkApplication is signaled to close the stream gracefully - i.e. return io.EOF and delete the stream from the map.
If context is Done() before Close() is called, the ctx.Err() is returned and the stream is deleted from the map.

After the stream is closed(either via Close() or via ctx.Done()) all Close() calls will return immediately(since the channel is closed by LinkApplication).

NOTE, that Close() will only ever return after the stream is actually removed from the map

[johanstokking]

You don't have to explain me how it works because I can read Go code.

People have been writing custom trigger and time-based cancelation code for 7 years before context.Context became a standard library in 1.7.

Now, I very much prefer using that, because it solves exactly the problem at hand. In fact, it's much simpler because you wait for one context to close instead of switching on some weird channel.

For the record, this is an open source project, and I like to keep code accessible for the community to see what's going on and contribute.

But if you believe that rolling custom stuff for obvious things that are in standard libraries is better or simpler (I disagree on both counts), fine.

[johanstokking]

1. Consistent component naming, Application Server
2. else for Debug log below

[rvolosatovs]

@johanstokking I simply do not understand how to implement following semantics using
context.Context:

• Close() could be called multiple times(possibly concurrently) safely
• Close() only returns once the stream is deleted from the map
• Close() returns instantly if stream is already closed
• LinkApplication is responsible for all the housekeeping, i.e. deleting/adding the strteam to the map

context.WithCancel can be used to cancel the context and trigger the "closing sequence" in LinkApplication, but there's no way to actually wait for LinkApplication in Close() to finish all the "housekeeping" if only one context is used.

[htdvisser]

Looks ok to me. Are you sure you want to merge all the added debug logs?

[htdvisser]

Inconsistent with debug log above, which uses - close link

[johanstokking]

Even though it's Debug I would not print code

[htdvisser]

For the record, I think @rvolosatovs's points justify this "custom" cancelation mechanism. Especially the requirement Close() only returns once the stream is deleted from the map is one that Go doesn't solve. Cancelation is easy, but waiting for cleanup after cancelation is currently impossible with context.Context.

From https://dave.cheney.net/2017/08/20/context-isnt-for-cancellation:

[...] context.Context‘s most important facility, broadcasting a cancellation signal, is incomplete as there is no way to wait for the signal to be acknowledged.

[johanstokking]

context.WithCancel can be used to cancel the context and trigger the "closing sequence" in LinkApplication, but there's no way to actually wait for LinkApplication in Close() to finish all the "housekeeping" if only one context is used.

Do you need to wait for that though? Is it even desirable to wait for that? If so, why?