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

http upload not working in version 4.1.0 if conversations push proxy is used #1410

Closed
ChaosKid42 opened this issue Jan 12, 2019 · 2 comments

Comments

@ChaosKid42
Copy link
Contributor

commented Jan 12, 2019

http upload is not working (paperclip icon not shown) in version 4.1.0 with my own server (ejabberd 18.12.1). There's the following line on the console:

ERROR: <iq xmlns="jabber:client" from="p2.scholzbande.de" xml:lang="de" to="csztest@scholzbande.de/converse.js-65020540" type="error" id="0c3f22c0-4b87-4148-ab2d-590276e8383f:sendIQ"><error type="cancel"><service-unavailable xmlns="urn:ietf:params:xml:ns:xmpp-stanzas"/></error></iq>

p2.scholzbande.de is the conversations push proxy (https://github.com/iNPUTmice/p2).

The problem is due to discovery of http-upload feature failing and was introduced by commit 01f0a65.

After reverting this commit everything is fine and http upload works again.

@jcbrand jcbrand added this to the 4.1.1 milestone Jan 13, 2019

@ChaosKid42

This comment has been minimized.

Copy link
Contributor Author

commented Jan 13, 2019

@jcbrand After thinking about it for a while, I believe it would be best to simply revert commit 01f0a65 as it makes no sense to reject the waitUntilFeaturesDiscovered promise only because a service discovery request on a single service fails. converse.js should simply continue and query the next service as other clients (e.g. gajim) seem to do it.
I'm not even sure if the push service is to blame. Maybe it's completely legitimate for a service to not support discovery. Anyway, clients should be able to cope with such a situation.

@jcbrand

This comment has been minimized.

Copy link
Member

commented Jan 14, 2019

it makes no sense to reject the waitUntilFeaturesDiscovered promise only because a service discovery request on a single service fails.

Previously, when the IQ request failed but we resolved instead of rejected the promise, then an empty Collection of features was created.

This was a problem for MUCs, because the MUC might not exist at that point yet, but will then be created, in which case Converse wrongly thinks the MUC has zero features.

So my initial idea was to reject the promise and then have proper error handling. I ended up solving the MUC issue a bit differently, but I still think rejecting makes more sense semantically and that the problem lies in the error handling instead.

clients should be able to cope with such a situation.

Yes, but my current thinking is that Converse should deal with this via better error handling of the rejected promise, not by pretending that all is fine and creating an empty Features Collection which can cause confusion and false reporting down the road.

jcbrand added a commit that referenced this issue Feb 13, 2019

@jcbrand jcbrand closed this Feb 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.