Closed evanweible-wf closed 7 years ago
Does it work when you compile to js?
@kevmoo Yes, looks like it does. Tests pass in Chrome, Firefox, and Safari. So I guess that might narrow it down to a Dartium issue.
@evanweible-wf If we checkout your repository, is there a minimal repo we can run to validate?
@kevmoo here you go:
https://github.com/ekweible/dart_1.20.1_w_transport_future_bug_repro
If you clone that, you can see the failure by running the test in either content-shell or dartium:
$ pub run test -p content-shell
$ pub run test -p dartium
Running in any other browser, the test will pass:
$ pub run test -p chrome
@lrhn @floitschG Would you guys take a look? – also confirmed on 1.21.0-dev.4.0
The class being mentioned is CommonPlainTextRequest&BrowserRequestMixin.abortRequest
which is a class created using a mixin in w_transport/lib/src/http/browser/requests.dart
as CommonPlainTextRequest with BrowserRequestMixin
. That library doesn't import dart:async
.
It seems like the VM is messing up the type lookup because it can't find the Future
class there. The error is reported at line 55 which is the end of the file, so it suggests a problem with synthetic code, not actual code.
Now, it shouldn't be looking there at all, it should be doing static lookup relative to where the mixin is declared, and dart:async
is imported in that library (request_mixin.dart
in the same dir).
I'm not sure why this only happens in dartium/content-shell. One hint is that the stack trace starts with:
dart:html HttpRequest.abort
but I have no idea why it matters.
I think it's safe to say that it's a VM issue of some sort.
Can someone point me to a x64 debug build of content_shell? Or even better, to a recipe for building it myself?
Not having a debug build of content_shell, I used the latest version of dart_bootstrap to compile the test case. Tracing type finalization shows that 'Future' is being finalized for class 'BrowserRequestMixin', and not for class 'CommonPlainTextRequest&BrowserRequestMixin', which would cause the problem.
I related bug was fixed on September 19. Could it be that content_shell/dartium are using an older dart VM?
Interestingly, if I bypass pub and use content_shell directly with an index.html pointing to w_transport_future_bug_test.dart, I get "00:00 +1: All tests passed!"
@crelier success w/out pub might be due to the code exiting before the async error is thrown...
@crelier any updates here?
Debugging on this problem has been slow because of various issues with building a debug version of content shell. I believe Regis managed to get a debug build after various tries only this afternoon.
Yes, I learned the hard way that our older version of dartium/content_shell cannot be built out of the box anymore without manually fixing depot_tools. Another complication is that the issue does not occur if content_shell is ran by itself, but only when invoked by the test runner, which requires modification in order to launch a debug build of content_shell that can be inspected under gdb. It is a very long story, but in short, no update yet.
After the fix, the test sometimes succeeds and sometimes fails in a way unrelated to the issue of this report (timeout?):
$ pub run test -p content-shell 00:03 +1: All tests passed!
$ pub run test -p content-shell
00:03 +0 -1: test/w_transport_future_bug_test.dart: w_transport Future not resolved repro
RequestException: GET 0 http://httpstat.us/200
[object XMLHttpRequestProgressEvent]
package:w_transport/src/http/browser/request_mixin.dart 85:43 CommonPlainTextRequest&BrowserRequestMixin.sendRequestAndFetchResponse.
00:03 +0 -1: Some tests failed.
I discovered this issue via the w_transport package when upgrading from Dart 1.19.1 to 1.20.1. All tests pass on 1.19.1, and then without any changes other than upgrading, there are a few tests that fail with the following error:
To start, it helps to understand the structure of the w_transport code involved in this failure.
The main entry point (
w_transport/w_transport.dart
) exposes some abstract request classes that use factory functions to construct the correct implementations based on the configured platform (browser vs VM). I'll useRequest
as the example here.https://github.com/Workiva/w_transport/blob/master/lib/src/http/requests.dart#L197-L220
There are then two platform-specific entry points for browser and VM. The consumer has to choose one to import and configure so that the platform-independent code knows how to construct the correct concrete implementations at runtime.
Most of the logic is shared between request types and between platforms, so there's an internal common class to house it all.
https://github.com/Workiva/w_transport/blob/master/lib/src/http/common/request.dart#L38
To fill in the gaps specific to each platform, I use a mixin.
https://github.com/Workiva/w_transport/blob/master/lib/src/http/browser/request_mixin.dart#L27
Finally, a concrete version of the request is created by extending the common request class and mixing in a platform-specific one.
https://github.com/Workiva/w_transport/blob/master/lib/src/http/browser/requests.dart#L26
To bring it full circle.. if a user of w_transport had configured the platform to be "browser", then constructing a new instance of the abstract
Request
class would result in that factory using a browser-specific factory that constructs an instance ofBrowserRequest
.Now back to the failure that was seemingly introduced due to a change in Dart 1.20. The file where I define all of the concrete browser request classes (https://github.com/Workiva/w_transport/blob/master/lib/src/http/browser/requests.dart#L26) is relatively small - it just imports other classes and extends/mixes them to create the desired result. Because of this, there is no
dart:async
import in that file (there's no logic that depends on it).I mention this because the stack trace from the failure above points to this file, but it points to the very last line (which is blank), and if I add
import 'dart:async';
to that file, the failure no longer occurs. And then the analyzer complains that the import is unused.This also seems to be related to the
HttpRequest
class fromdart:html
somehow. The only tests that fail are those that are exercising one of two things:.abort()
on an instance ofdart:html.HttpRequest
onAbort
stream on an instance ofdart:html.HttpRequest
The exact same pattern is followed for the VM implementations (using
dart:io
), but those are still working as expected.I'll keep trying to come up with a reduced test case, but so far it seems to be the result of the way I'm using abstract classes and mixins to work around platform-specific dependencies (like
dart:html
) and I haven't been able to simplify that much without losing the failure.In the mean time, if anyone has some ideas or suspicions about what might be happening here, please let me know!