[
https://issues.apache.org/jira/browse/METRON-1790?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16646146#comment-16646146
]
ASF GitHub Bot commented on METRON-1790:
----------------------------------------
Github user ruffle1986 commented on a diff in the pull request:
https://github.com/apache/metron/pull/1208#discussion_r224366897
--- Diff:
metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts
---
@@ -81,26 +81,28 @@ export class PcapPanelComponent implements OnInit,
OnDestroy {
this.pdml = null;
this.progressWidth = 0;
this.errorMsg = null;
- this.submitSubscription =
this.pcapService.submitRequest(pcapRequest).subscribe((submitResponse:
PcapStatusResponse) => {
- let id = submitResponse.jobId;
- if (!id) {
- this.errorMsg = submitResponse.description;
- this.queryRunning = false;
- } else {
- this.startPolling(id);
+ this.subscriptions['submitSubscription'] =
this.pcapService.submitRequest(pcapRequest).subscribe(
+ (submitResponse: PcapStatusResponse) => {
+ let id = submitResponse.jobId;
+ if (!id) {
+ this.errorMsg = submitResponse.description;
+ this.queryRunning = false;
+ } else {
+ this.startPolling(id);
+ }
+ }, (error: any) => {
+ this.errorMsg = `Response message: ${error.message}. Something
went wrong with your query submission!`;
--- End diff --
In case of `statusSubscription`, we have to unsubscribe because we want to
stop polling the server if an error occurs.
But the other observables are one-time observables which means once they
complete (or throw an error) we won't get any additional event until we
subscribe again. So in theory, if would be very precise, we unsubscribed from
the observables in case of fulfilling not just in case of errors. But it would
lead to code that is hard to reason about.
So yes, it's a nice catch. It's a bit more verbose to unsubscribe from the
cancelSubscription because it's a one-time observable too. It's not bad, but
it's imperative and the goal of using observables to write less code following
a declarative approach.
Changing the way how you think in order to get the most out of observables
is hard. It's hard even for me. Here we perform a lot of http requests using
observables because we need it to meet the business requirements (feature) and
that's angular way right? But we're doing it wrong. When it comes to multiple
observables in one component, we should compose them somehow together in order
the take advantage of reactive programming (RxJs). But you have to be very
careful with composing observables because it's really hard to get it done or
even maintain it later or read and understand what the code intends to do.
Long story short, when an http observable fulfils, it's not necessary to
unsubscribe. There I can remove the `unsubscribe` from the cancel event if an
error occurs in order to avoid confusion.
The goal of this PR was to cancel the actual http requests when the user
navigates somewhere else. If users are not interested in the pcap panel
anymore, we don't need to wait for the server's answer. It's silly to wait for
the answer for a pcap request when you're on the alerts page.
I'm going to remove the unsubscribe from the cancel observable to avoid
confusion.
I highly recommend to read this article by Ben Lesh (lead dev of the RxJs
team) about it:
https://medium.com/@benlesh/rxjs-dont-unsubscribe-6753ed4fda87
> Unsubscribe from every observable in the pcap panel UI component
> ----------------------------------------------------------------
>
> Key: METRON-1790
> URL: https://issues.apache.org/jira/browse/METRON-1790
> Project: Metron
> Issue Type: Improvement
> Reporter: Tamas Fodor
> Assignee: Tamas Fodor
> Priority: Minor
>
> There are a lot of http requests performed in the pcap panel ui component and
> we just unsubscribe from some of them when the component is no longer
> rendered on the screen. It could cause memory consumption issues. Because of
> the active subscriptions, the garbage collector is not able to remove these
> objects from the memory, however they're not needed to be there anymore.
> There's another benefit of unsubscribing from these http calls. If the user
> leaves the pcap tab but there are pending requests, the unsubscribe method
> cancels the active xhrs immediately so it won't wait for fulfilment
> unnecessarily.
> [https://github.com/apache/metron/blob/master/metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts#L54]
> I would also refactor that part when we convert an observable to a promise. I
> would keep it as an observable. By doing this, we would be able to
> unsubscribe from it as well in the destructor method. Promises are not
> cancelable.
> [https://github.com/apache/metron/blob/master/metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts#L70]
> Resources:
> [https://angular.io/guide/lifecycle-hooks#ondestroy]
> This is the place to free resources that won't be garbage collected
> automatically. Unsubscribe from Observables and DOM events. Stop interval
> timers.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
