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

METRON-1790: Unsubscribe from every observable in the pcap panel UI component #1208

Closed
wants to merge 2 commits into from
Closed

METRON-1790: Unsubscribe from every observable in the pcap panel UI component #1208

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b2fd1b9
feat(pcap panel): unsubscribe from every observable.
ruffle1986 Sep 21, 2018
50787b3
refactor: remove unnecessary unsubscribe
ruffle1986 Oct 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 32 additions & 36 deletions metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,40 +35,40 @@ export class PcapPanelComponent implements OnInit, OnDestroy {
pdml: Pdml = null;
pcapRequest: PcapRequest;
resetPaginationForSearch: boolean;

statusSubscription: Subscription;
cancelSubscription: Subscription;
submitSubscription: Subscription;
getSubscription: Subscription;
queryRunning = false;
queryId: string;
progressWidth = 0;
pagination: PcapPagination = new PcapPagination();
savedPcapRequest: {};
errorMsg: string;
cancelConfirmMessage = 'Are you sure want to cancel the running query?';
subscriptions: {
[key: string]: Subscription
} = {};

constructor(private pcapService: PcapService) { }

ngOnInit() {
this.pcapRequest = new PcapRequest();
this.pcapService.getRunningJob().subscribe((statusResponses: PcapStatusResponse[]) => {
this.subscriptions['runningJobSubscription'] = this.pcapService.getRunningJob().subscribe((statusResponses: PcapStatusResponse[]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we never see an unsubscribe for 'runningJobSubscription'?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do here: https://github.com/ruffle1986/metron/blob/METRON-1790/metron-interface/metron-alerts/src/app/pcap/pcap-panel/pcap-panel.component.ts#L138-L143

I collect every subscription in an object and I go through all of them in the unsubscribeAll method.

if (statusResponses.length > 0) {
// Assume the first job in the list is the running job
this.queryRunning = true;
let statusResponse = statusResponses[0];
this.updateStatus(statusResponse);
this.startPolling(statusResponse.jobId);
this.pcapService.getPcapRequest(statusResponse.jobId).subscribe((pcapRequest: PcapRequest) => {
this.pcapRequest = pcapRequest;
});
this.subscriptions['pcapRequestSubscription'] = this.pcapService.getPcapRequest(statusResponse.jobId).subscribe(
(pcapRequest: PcapRequest) => {
this.pcapRequest = pcapRequest;
}
);
}
});
}

changePage(page) {
this.pagination.selectedPage = page;
this.pcapService.getPackets(this.queryId, this.pagination.selectedPage).toPromise().then(pdml => {
this.subscriptions['packetSubscription'] = this.pcapService.getPackets(this.queryId, this.pagination.selectedPage).subscribe(pdml => {
this.pdml = pdml;
});
}
Expand All @@ -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!`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not need to unsubscribe here like we do for 'statusSubscription' and 'cancelSubscription'?

this.subscriptions['submitSubscription'].unsubscribe();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

What happens if I submit a pcap request, get bored and navigate back to Alerts, then navigate back to Pcap to check on my job status? Will I be able to 'see' my job finish and view the pcap?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes because it's already implemented. Every time the pcap panel is rendered (ngOnInit ) we ask the server whether we have a running job in the background. If so, we reattach the job on the UI and keep pooling the server until the job gets done. But it has nothing to do with what we're doing here in this PR.

}
}, (error: any) => {
this.errorMsg = `Response message: ${error.message}. Something went wrong with your query submission!`;
});
);
}

startPolling(id: string) {
this.queryId = id;
this.errorMsg = null;
this.statusSubscription = this.pcapService.pollStatus(id).subscribe((statusResponse: PcapStatusResponse) => {
this.subscriptions['statusSubscription'] = this.pcapService.pollStatus(id).subscribe((statusResponse: PcapStatusResponse) => {
this.updateStatus(statusResponse);
}, (error: any) => {
this.statusSubscription.unsubscribe();
this.subscriptions['statusSubscription'].unsubscribe();
this.queryRunning = false;
this.errorMsg = `Response message: ${error.message}. Something went wrong with your status request!`;
});
Expand All @@ -109,9 +111,9 @@ export class PcapPanelComponent implements OnInit, OnDestroy {
updateStatus(statusResponse: PcapStatusResponse) {
if ('SUCCEEDED' === statusResponse.jobStatus) {
this.pagination.total = statusResponse.pageTotal;
this.statusSubscription.unsubscribe();
this.subscriptions['statusSubscription'].unsubscribe();
this.queryRunning = false;
this.pcapService.getPackets(this.queryId, this.pagination.selectedPage).toPromise().then(pdml => {
this.subscriptions['packetSubscription'] = this.pcapService.getPackets(this.queryId, this.pagination.selectedPage).subscribe(pdml => {
this.pdml = pdml;
}, (error: RestError) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we not need to unsubscribe here like we do for 'statusSubscription' and 'cancelSubscription'?

this.subscriptions['packetSubscription'].unsubscribe();

if (error.status === 404) {
Expand All @@ -121,7 +123,7 @@ export class PcapPanelComponent implements OnInit, OnDestroy {
}
});
} else if ('FAILED' === statusResponse.jobStatus) {
this.statusSubscription.unsubscribe();
this.subscriptions['statusSubscription'].unsubscribe();
this.queryRunning = false;
this.errorMsg = `Query status: ${statusResponse.jobStatus}. Check your filter criteria and try again!`;
} else if (this.progressWidth < 100) {
Expand All @@ -134,25 +136,19 @@ export class PcapPanelComponent implements OnInit, OnDestroy {
}

unsubscribeAll() {
if (this.cancelSubscription) {
this.cancelSubscription.unsubscribe();
}
if (this.statusSubscription) {
this.statusSubscription.unsubscribe();
}
if (this.submitSubscription) {
this.submitSubscription.unsubscribe();
}
Object.keys(this.subscriptions).forEach((key) => {
this.subscriptions[key].unsubscribe();
});
this.subscriptions = {};
}

cancelQuery() {
this.cancelSubscription = this.pcapService.cancelQuery(this.queryId)
this.subscriptions['cancelSubscription'] = this.pcapService.cancelQuery(this.queryId)
.subscribe(() => {
this.unsubscribeAll();
this.queryId = '';
this.queryRunning = false;
}, (error: any) => {
this.cancelSubscription.unsubscribe();
this.queryId = '';
this.errorMsg = `Response message: ${error.message}. Something went wrong with the cancellation!`;
this.queryRunning = false;
Expand Down