Mithril.js: m.request don't catch invalid domain errors

Created on 8 Mar 2017 · 11Comments · Source: MithrilJS/mithril.js

In version 1.0.1, when m.requests() is called with an incorrect URL, the promise returned stays indefinitely in the pending state.

Code to reproduce:

<!doctype html>
<html>
    <body>
        <script src="https://unpkg.com/mithril/mithril.js"></script>
        <script>
            console.log('Version', m.version);
            console.log('Before request');
            var p = m.request({
                url: 'http://not-exists'
            });
            console.log('Request sent', p);

            p.then(function(result) {
                console.log('Promise resolved', result);  // Never called
            }).catch(function(error) {
                console.log('Promise failed', error);  // Never called
            });
        </script>
    </body>
</html>

From @orbitbot :

the issue is that this line : https://github.com/lhorie/mithril.js/blob/next/request/request.js#L107 is outside of a try/catch block and leaves the promise hanging...
which seems to be where at least undefined URLs end up

Bug

Source

BAKFR

👍2

All 11 comments

Playing around with the sample code in FF & Chrome for a while, I've noted that

this could in theory be caught by detecting status 0 and readystate 4, however from looking at the commit history that seems to be the same case as when manually aborting the request, adressed in https://github.com/lhorie/mithril.js/commit/9558c8e2e91c956710aa54be1960ce4699fced20 https://github.com/lhorie/mithril.js/commit/a640eaeb44e297e9667e29ea4e015e822a7c2012
FF does not throw anything at all on xhr.send, but xhr.onerror is fired with a ProgressEvent with type: "error" in both Chrome & FF
it's not possible to handle the case with setting a XHR timeout on either FF or Chrome, since the request is in fact not ever sent
Mithril doesn't have an ergonomic way of setting and handling the XHR timeout, could be handled on the opts argument similarly to headers
even if you set custom timeouts, there's no way of resolving or rejecting the returned promise from the original m.request call, since it's not accessible in the config function
the docs should explicitly state that you need to _return_ the modified XHR in the config method
m.request should arguably care a bit about the xhr.events by default
tangentially: looking at the spec, xhr.open can actually also throw in some cases

All in all, this particular case could be fairly trivially fixed, but perhaps there is a reason to discuss if there are some reasonable differentiator between what m.request actually should handle as an API and what is left to the user, since functionality can be significantly different if Mithril attaches and tries to handle all the XHR event handlers by default.

Slightly edited test code: http://jsbin.com/penesegovu/edit?html,console

Here's a discussion on jQuery's solution a few years back https://bugs.jquery.com/ticket/14683

orbitbot on 8 Mar 2017

When a connection refused error occurs in Chrome, a stack trace is shown that makes it look like the exception happened synchronously from xhr.send()

However, you can't catch it by surrounding your call to m.request with a try catch block. xhr.onerror DOES get called. I suppose it would be the perfect world if that case causes the response Promise to be rejected. That screenshot is from:

https://jsbin.com/panulimolo/edit?js,console

arienkock on 10 Mar 2017

👍1

@arienkock the reason you can't catch errors relating to high level HTTP failure is that it poses security risks, inasmuch as malicious code could determine a user's access to various services and resources.

As you point out, this is a browser feature — not something the library can address.

barneycarroll on 16 Mar 2017

@barneycarroll Fair enough on the security issues (though admittedly I'm not entirely sure I grok the reasoning), but the returned promise being rejected if the request fails for any reason seems like the principle of least surprise to me. Axios and the Fetch API handle high level errors (that is errors not relating to the server's response) by rejecting the promise. https://jsbin.com/gehoboxuma/edit?js,console

arienkock on 16 Mar 2017

IMO (with some afterthought, but there may be edge cases that I haven't considered), m.request should natively

reject the promise when xhr.onerror fires (if aborts end up here, then maybe that's one case when it doesnt?)
reject the promise when xhr.ontimeout fires
support opts.timeout that just sets xhr.timeout = opts.timeout
minimally, expose the internal promise as f.e. the third parameter to opts.config

With the above, I'm not sure to what degree the implementation needs to care about readyState changes, or if there are _additional_ cases that require readyState observation for something or the other. @barneycarroll This is one example of where a Litmus project would be awesome for library writers...

The first two suggestions are not particularly difficult to implement, but already major version bump material. If someone for some reason feels that this is too much or this should rather be part of a larger release of multiple things, then at least exposing the promise would allow consumers to hook stuff like the other three suggestions in a minor version bump.

Disclaimer: just skimmed parts of the XHR2 spec, and haven't looked at the source of other XHR wrappers to see what cases they're handling or not.

orbitbot on 16 Mar 2017

@arienkock had posted a snippet to wrap m.request so that it rejects if xhr.onerror fires, I see it in my mail but not here... There was AFAICT a small bug, though (missing , reject as a then argument).

var m = require("mithril");

function createOptions(reject) {
  return {
    config: function(xhr) {
      xhr.onerror = reject
    }
  }
}

module.exports = function(options) {
  return new Promise(function(resolve, reject) {
    m.request(Object.assign({}, options, createOptions(reject))).then(resolve, reject);
  });
}

pygy on 17 Mar 2017

@pygy Hah, yes, I fixed that, but then I removed it after all as I suspected there was another bug in it. So far that version you posted seems to work with one caveat: It seems like m.redraw() needs to be called in any downstream rejection handlers.

arienkock on 17 Mar 2017

👍1

Moving out of the 1.1.0 milestone since movement on this has slowed.

tivac on 24 Mar 2017

👍1

@orbitbot For those, here's my thoughts:

reject the promise when xhr.onerror fires (if aborts end up here, then maybe that's one case when it doesnt?)

It seems odd that Mithril wouldn't reject here, to the point I'd consider it a bug.

reject the promise when xhr.ontimeout fires

support opts.timeout that just sets xhr.timeout = opts.timeout

I'd be okay with that if and only if opts.timeout is set. (If it's set via opts.config, let the user handle it.)

minimally, expose the internal promise as f.e. the third parameter to opts.config

If by that, you mean resolve/reject, that could work for more advanced use cases. (If you need any post-request cleanup work, just use the return value.)

isiahmeadows on 15 Jun 2017

If by that, you mean resolve/reject, that could work for more advanced use cases.

Yeah, that was the intent. I keep forgetting that native promises didn't end up with the deferred API for some reason :smile:

orbitbot on 15 Jun 2017

Can't duplicate this in mithril@next, so closing.

https://flems.io/#0=N4IgZglgNgpgziAXAbVAOwIYFsZJAOgAsAXLKEAGhAGMB7NYmBvEAXwvW10QICsEqdBk2J4hcWrHxRaAcwAUAcgBCMMLQBOMAARaAjgFd4xRQEoA3AB00ANwwbtAB20BebVnz6jcYvODXtQO0DDShEbUUSYkdEAHpYtFpiAFoYAA8IHzhFa1YLa3FJGGk5JQAlGENjbTgRRQonfLRrR3xiQiZ5MAM0amIIenktOAMoYlNtfzQg7UKpGQVFAAUNWixMnWHJGxgAE3rdeFHx80D47QA5GB2HagwoWF3c03w74mpCLp6+gbR5GA0qw0EymMzmxQWShWaw22jAGGgewOAKBFjOsUu1wBs3uj2e5jYHBAmBweFecAENHojGYPDYAF0qFAIGgANYIFCcUk8dbtDTQAACaHSoioIXIPCijjgcViPUcrNkrzWsV5hH5UAFACZ8AAGPXJDTUfAARlVED50Hw-EoIGIAE9HNwQHBqPzHKJWPTWEA

isiahmeadows on 7 Nov 2018

Was this page helpful?

0 / 5 - 0 ratings