Don't erase well known data when the fetch fails

[chagai95]

Signed-off-by: Chagai Friedlander [email protected]

---

This PR currently has none of the required changelog labels.

A reviewer can add one of: T-Deprecation, T-Enhancement, T-Defect, T-Task to indicate what type of change this is, or add Type: [enhancement/defect/task] to the description and I'll add them for you.

[t3chguy]

The well known file is allowed to be deleted. That should have the same effect as creating a file without any options within it {}. With this change, if the file was to be deleted then that change would not be reflected in clients until they restart.

[chagai95]

The well known file is allowed to be deleted. That should have the same effect as creating a file without any options within it {}. With this change, if the file was to be deleted then that change would not be reflected in clients until they restart.

Isn't that better than having the default values being used instead of the well known? I think currently every time the connection is not stable this results in the well known being deleted. Maybe it's better to have admins actively upload an json in order to actually "delete" the well known... Which is probably very rare anyway, is it not?

[t3chguy]

Isn't that better than having the default values being used instead of the well known?

They're both incorrect

Maybe it's better to have admins actively upload an json in order to actually "delete" the well known...

It'd have to be a suggestion codified into the spec for us to rely on this

[chagai95]

Just for the record, I'm leaving this here because I think it's the right thing to do. Aarenet will have this on their fork, and I suggest you also do this. Currently, your setup isn't worried too much about these problems because all the fallbacks are the same in the config anyway and even hard coded, but this is a very big issue for us, and I'm sure for many others that will be using the sdk this way (a generic client meant for use by different servers).

I think it's not really necessary to spec this before changing it, because like you mentioned they are both incorrect and there is no specific mention of this in the spec as far as I can tell (am I wrong? 🙈)

This becomes even more difficult to handle using authentication with Jitsi, which is what we do, and we end up having the wrong Jitsi domain just because some client didn't have an internet connection at a certain point (and removing the widget is deliberately not a possibility on our setup, so the user has to get rid of the room).

[t3chguy]

because like you mentioned they are both incorrect and there is no specific mention of this in the spec as far as I can tell (am I wrong? 🙈)

If the returned status code is 404, then IGNORE.
If the returned status code is not 200, or the response body is empty, then FAIL_PROMPT.

https://spec.matrix.org/v1.3/client-server-api/#well-known-uri

[chagai95]

Yes, but I'm talking about the polling, there is no mention of how often you should poll or even if you should, polling every two hours and overriding the previous values is prone to a lot of cases where the fallback will be used for no reason. Also a 404 or not a 200 is not exactly the same as the user not having an internet connection, or am I wrong? Maybe polling should only be done when the user has internet connection?

In the network tab in the browser I can see failing and not 404, but maybe there's a convention I'm not aware of...

[t3chguy]

Maybe polling should only be done when the user has internet connection?

That sounds sane but there's no standard way to do that in all JS environments, the web has online/offline emits but Node doesn't

In the networking tab in the browser I can see failing and not 404, but maybe there's a convention I'm not aware of...

Yup, worse yet in the web the browser will hide the underlying failure behind a CORS error

[dbkr]

Hey, sorry this has gone ignored for so long. We were trying to get our heads around this and it feels like the problem might be more to do with the function returning empty well-knowns on failure, so although this does seem like a problem, I'm not sure this is necessarily the right solution, since it seems valid to have an empty well known file?

[chagai95]

Hey Dave, no worries, there are more important stuff to deal with, but thx for bringing it up again 🙂

I'm not sure what the consequences of this will be, but I'm assuming that deleting or setting the JSON to an empty array with the intention of overriding it is more rare than losing internet connection and even when that inevitably happens, it should be solved by a client restart?

What consequences are you worries about? I'm aware this is not a perfect solution but I was thinking it might be slightly better than what is happening now and I'm not sure how it could be done better without a lot more work...

[dbkr]

Just brought this up again in our meeting. I don't think there's a specific thing we're worries about, it's more a case of wanting to understand exactly what's going on here a bit better. The checklist of things, ideally, would be:

• What actual problems are caused by losing the well-known data? (ie this doesn't refer to a bug its fixing)
• Ideally, distinguish between 500 errors etc and actual empty well-known files
• Tests would be great for this too

[dbkr]

Closing for now so it doesn't keep coming up, but feel free to reopen / open a new one taking the above into consideration.