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.

Sign up for GitHub

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

Use events to wire upload tracking instead of racing with setImmediate #525

Closed
wants to merge 1 commit into from
Closed

Use events to wire upload tracking instead of racing with setImmediate #525

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 7 additions & 11 deletions source/request-as-event-emitter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,11 @@ module.exports = (options = {}) => {
return;
}

setImmediate(() => {
try {
getResponse(response, options, emitter, redirects);
} catch (error) {
emitter.emit('error', error);
}
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this was added to control the call stack, ensuring that the method ran to completion before any errors were emitted. This changes that behavior. There doesn't appear to be any tests failing from the change, but I would guess that this introduces behavioral changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally added in #117.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brandon93s Thanks for the reference to #117.

It looks like the intent was to ensure the response data was handled after the event listeners that clear timeouts were run.

If a callback is provided for request, it will be the first handler to run.

I can make another pass at this, to evaluate when the timeout clearing handlers are being run.

Does my analysis sound right? See any other issues?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You both are right. The main intent was to emit response ASAP, so @floatdrop used setImmediate as it executes immediately after I/O operations. I'd leave setImmediate unchanged here.

If a callback is provided for request, it will be the first handler to run.

I think @jstewmon is right here. We don't need setImmediate in this case (line 198).

Copy link
Contributor Author

@jstewmon jstewmon Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main intent was to emit response ASAP

That's not how I read it. There are 3 emitters involved, so I'm not sure which one you mean. The emitter response event is emitted from getResponse, so setImmediate here delays that. The req emitter's event isn't affected by this setImmediate.

This is the background information I found:

The version of timed-out being used at the time of #117 listened for the response event of the request to clear the timeout:
https://github.com/floatdrop/timed-out/blob/v2.0.0/index.js#L31

The request object was passed to timed-out, which would setup that listener.

The statement that was deferred with setImmediate called unzipResponse from the http.request callback, which would have run before timed-out's listener.

So my assessment was that the intent was to allow timed-out's listener to run before the response stream was connected to unzipResponse.

Now that timeout.request is waiting for response.end, I think deferring the call to getResponse actually increases the possibility that the timeout will breach.

Since it is quite common to use a request callback, I think a better solution to checking a timeout against the response event would be to use setImmediate in the timer callback to defer the error emission to the check phase of the event loop in case the response had been received but was still queued due to load. At a minimum, the listener that clears the timeout could be subscribed with prependOnceListener to ensure it runs earlier.

I thought there might be some practical purpose other than the ordering of the response handlers (like allowing subscriptions to some event emitter), but I don't see any such purpose, and I think we can clearly see that the response event listener order is no longer applicable.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a better solution to checking a timeout against the response event would be to use setImmediate in the timer callback to defer the error emission to the check phase of the event loop in case the response had been received but was still queued due to load.

So the connection was established, because there's a response.

Here's some code to illustrate what I meant about using setImmediate with timers.

You were trying to illustrate the first sentence using the connect timeout, though it had been connected already. Awesome.

Is it possible to receive connect event and response (with some queued data) in the same loop?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you carefully reviewed the doc I linked earlier about the event loop phases?

Yes, I have read it several times, not only today.

The point is that if we're checking connect timeout. The connection may have already been established, but its event hasn't fired when the timeout callback is run.

Then the connection is not established. From the docs:

Event: 'connect'
Emitted when a socket connection is successfully established.

socket.connecting
If true - socket.connect(options[, connectListener]) was called and haven't yet finished. Will be set to false before emitting 'connect' event and/or calling socket.connect(options[, connectListener])'s callback.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a tricky topic because of the changes in behavior over time.

When I've referred to the response event, I've been talking about the behavior and intent at the time (circa 2015) that the setImmediate deferral was added in #117. The response event is not currently used to clear any timeouts, so it is not relevant to this change.

You were trying to illustrate the first sentence using the connect timeout, though it had been connected already.

No, the statement you quoted was in my original analysis. I was suggesting that there was a more precise way of mitigating #117 than deferring the response piping with setImmediate.

I didn't want to use the response event to illustrate how that suggestion would be implemented because none of the current timeouts depend on the timing of that event. I used the connect event as an example in the current code because it is used to clear the connect timeout.

Is it possible to receive connect event and response (with some queued data) in the same loop?

I don't see any reason why that would not be possible, but their listeners would be notified in FIFO order (connect before response). Does it matter?

Copy link
Contributor Author

@jstewmon jstewmon Jul 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the connection is not established

The underlying system operation completes before we're notified. That's the purpose of the poll phase - to notify us that the IO operations we listened for are complete.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that timeout.request is waiting for response.end, I think deferring the call to getResponse actually increases the possibility that the timeout will breach.

Not true. Before I've copied timed-out into this repo there was pTimeout handling timeout.request, so it started counting immediately after the promise was called (no connection has been made yet). That's just only a note.

The underlying system operation completes before we're notified. That's the purpose of the poll phase - to notify us that the IO operations we listened for are complete.

I still disagree here. It has its own time to establish the connection. But this change is not significant I think. Let's skip this subject.

No, the statement you quoted was in my original analysis. I was suggesting that there was a more precise way of mitigating #117 than deferring the response piping with setImmediate.

So if these are two different things, I've got only one question: what is the better way to avoid bottleneck with emitting the response and handling it? (Line 107) What else if not setImmediate? You haven't implemented it yet. It just delays establishing another connections for now (if there's big amount of requests).

try {
getResponse(response, options, emitter, redirects);
} catch (error) {
emitter.emit('error', error);
}
});

cacheReq.on('error', error => {
Expand All @@ -110,7 +108,7 @@ module.exports = (options = {}) => {

cacheReq.once('request', req => {
let aborted = false;
req.once('abort', _ => {
req.once('abort', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring should be done in a separate PR. Just Kidding :P 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered it, but when I looked at the diff, I didn't think the one line change raised the cognitive burden enough to warrant a separate commit. 😅

aborted = true;
});

Expand All @@ -133,9 +131,7 @@ module.exports = (options = {}) => {
timedOut(req, options.gotTimeout);
}

setImmediate(() => {
emitter.emit('request', req);
});
emitter.emit('request', req);
});
};

Expand Down