FlowChunk: xhr abort fixes #351
Conversation
| if (this.xhr) { | ||
| this.xhr.abort(); | ||
| } | ||
| this.xhr = null; |
There was a problem hiding this comment.
I remember this was made on purpose to prevent some kind of loop happening. I think abort throws some kind of synchronous event and that event calls abort there abort is called once again and it throws an error that it was already aborted or something.
There was a problem hiding this comment.
Could happen if something registered the abort event of the XHR itself:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort_event
But none of the code does this AFAICT. I'd say it's up to the consumer to attach "safe" event handler to XHR abort.
Don't you think?
There was a problem hiding this comment.
Since it solves your problem and we don't have any other explanation, let's leave it in a way it works best for you
src/FlowChunk.js
Outdated
| // on this FlowFile. | ||
| this.pendingRetry = false; | ||
| this.xhr = {readyState: 5, status: 1 }; | ||
| this.xhr = {readyState: 5, status: 200, abort: e => null }; |
There was a problem hiding this comment.
not sure how I missed this, but creating fake objects doesn't seem right. Other approach should be considered
There was a problem hiding this comment.
Sure. What do you suggest? Here we get a chunk and we identify, lately, that it's useless and we want to discard it without tainting the chunk/file status. (It's been completed)
There was a problem hiding this comment.
An alternative fake XHR:
this.xhr = new XMLHttpRequest();
this.xhr.open("GET", "data:");
this.xhr.send();A possible issue with this exists if a CSP is set but connect-src omits data:. (XHR would fail).
Do you see another safe solution?
As per status(), the two conditions (regarding the XHR) are clear: this.xhr.readyState must exist and be equal to 4 and this.xhr.status must be an accepted status.
There was a problem hiding this comment.
I just think we can rewrite this code part not to use fake xhr, because it might be used by other developers. It might raise some unexpected issues because it should be an instance of XMLHttpRequest but sometimes it's just an object. Just like in strongly typed languages, variable type should not change as it prevents some bugs.
Using this.xhr.open("GET", "data:"); does not seem right either, just a waste of resources and adds some complexity. Why do we have to have it defined? setting it to null or using other variable should work fine too imo.
There was a problem hiding this comment.
I totally agree with you concern, but
- By rewrite, do you mean rewrite
FlowChunk::status()? How? - Can we plan such a (probably deeper change) for a subsequent PR and keep this one focused on fixing
abort()? as that would allow me to move forward.
There was a problem hiding this comment.
Yes, we can merge as this is not really an issue of this MR
There was a problem hiding this comment.
How? Not sure, but it smells a bit in the current state
|
Approved |
2 participants
A couple of fixes regarding
abort()(observed in the particular case of a stream didn't compute adequately the number of chunks)(Note: An better codepath is still to be found to replace the current workaround used in
readStreamChunk)