Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in live version, cannot load multiple files by URL #54

Open
VandersandenMike opened this issue Aug 19, 2021 · 4 comments
Open

Bug in live version, cannot load multiple files by URL #54

VandersandenMike opened this issue Aug 19, 2021 · 4 comments

Comments

@VandersandenMike
Copy link

Describe the bug
Trying to load multiple files by URL results in an error.
Format 1 (?file=x.qlog) works like a charm, but when using format 4 (?file1=x.qlog&file2=y.qlog&file3=z.qlog) an error is thrown. The error also occurs when only trying to load a single file.

The error seems to be a bug.
Meanwhile, the inability to load multiple files seems to be a lacking feature. I quickly skimmed though the code, and it seems like only one file is attempted to be fetched, the others are ignored.

To Reproduce

  • load a file by URL parameter, using format 4: e.g. ?file1=http://localhost:5000/qlog/f49d5b63efe7abca.qlog
  • An error pops up saying: ERROR loading URL

Expected behaviour
When only supplying file1: same behaviour as format 1, correctly loading the file.
When supplying multiple files (file1, file2, ...): correctly loading all supplied files.

Error message

ConnectionStore.ts:302 ConnectionStore:LoadFilesFromServer : ERROR : trace not added to qvis! : {
file1: "http://localhost:5000/qlog/f49d5b63efe7abca.qlog"
}
Response: {
url: "http://localhost:5000/qlog/f49d5b63efe7abca.qlog%20etc."
}

Note that the response has %20etc. appended to the URL, which is absent in file1.

Possible fix
The error can be fixed using the following patch, which works for a local build:

diff --git a/visualizations/src/store/ConnectionStore.ts b/visualizations/src/store/ConnectionStore.ts
index fbce71b..9d3606a 100644
--- a/visualizations/src/store/ConnectionStore.ts
+++ b/visualizations/src/store/ConnectionStore.ts
@@ -136,7 +136,7 @@ export default class ConnectionStore extends VuexModule {
             urlToLoad = queryParameters.list;
         }
         else if ( queryParameters.file1 ){
-            urlToLoad = queryParameters.file1 + " etc.";
+            urlToLoad = queryParameters.file1;
         }
 
         if ( urlToLoad === "" ){
@JorisHerbots
Copy link

Qvis can't handle the above example because the files are hosted on localhost. Loading files using option 4 requires backend support. The addition of the %20etc. string actually makes the frontend download check fail. Perhaps not the most elegant way of doing this? 😛

The backend additionally merges the provided qlogs into one qlog containing the traces from the respective qlogs. I don't' think this is problematic in itself, but Qvis has no clear way of differentiating between the provided qlogs.

Two aioquic traces provided via option 4
Two aioquic traces provided via option 4

Two aioquic traces provided via option 1
The same two aioquic traces provided via option 1

A possible solution (the easiest one probably) would be to move the backend looping behaviour to the frontend and to perform a backend fetch request for each individual file if it can't be downloaded via de fetch API. This would allow local hosted files to be uploaded via option 4 and it would also solve the differentiation issue mentioned above at the cost of a couple more requests. I do, however, not know how the list parameter behaves. If it also returns a singular qlog with merged traces, then maybe we should be looking for a better way of differentiating traces if they originate from different qlogs?

sedrubal added a commit to sedrubal/quic-interop-runner-sat-web that referenced this issue Sep 29, 2021
@rmarx
Copy link
Member

rmarx commented Sep 30, 2021

So, this is a gift that keeps on giving... There are several issues in play at the same time here.
There is a partial fix in the commit above (and live on qvis.quictools.info), but I don't think this (fully) fixes things for either of you...

@sedrubal's problem was that adding "etc." produces a 404 instead of CORS error, which caused the fallback to the backend server to fail. However, I couldn't just remove the "etc." as @MikeV-1642883 suggested, because that would lead to just the "file1" being downloaded, and the rest ignored if that single file was directly downloadable (I've added comments to describe this).

This part is now fixed, but the next problem is that the files @sedrubal's trying to load are in the NDJSON format, which is currently not supported by the qvis backend, only the frontend... so the files now properly get sent to the backend, but it borks on the "malformed JSON". Since the NDJSON approach will be changed in the near future, I won't be adding that to the backend anytime soon. It should work though if you load qlog files not in NDJSON format (e.g., from aioquic instead of quic-go).

This fix doesn't do anything for @MikeV-1642883's multiple-localhost-files problem, which is more complex. To do this without backend fallback, I'd have to fully directly load multiple files in the frontend, and then also have logic on what to do if say 1 out of 5 files fails to download, if there is no CORS enabled on one of the files, etc. This is why I hadn't really solved this issue yet...

The commit above is quite dirty because I didn't have much time to properly refactor this. It's clear this entire setup needs a good overhaul sometime, but sadly I don't have time to do this atm.

@sedrubal
Copy link

sedrubal commented Oct 1, 2021

Thanks @rmarx for figuring that out 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants