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

request.response should be null when response failed to transmit #3728

Closed
PatrickQuintal opened this issue Jan 17, 2018 · 5 comments
Closed

request.response should be null when response failed to transmit #3728

PatrickQuintal opened this issue Jan 17, 2018 · 5 comments
Assignees
Labels
Milestone

Comments

@PatrickQuintal
Copy link

@PatrickQuintal PatrickQuintal commented Jan 17, 2018

Hello All,

I'm trying to detect whether or not the client connection was closed or aborted prematurely. Following the Hapi documentation (As well as the previous versions) it seems to have broken in 17.2.0.

Example;

I have a route that has this handler:

handler: function(request, h) {
			console.log("request");
			return new Promise(
				(resolve, reject) => {
					setTimeout(function() {
						console.log("response");
						return resolve(request.payload);
					}, 5000);
				},
				() => {}
			);
		},
server.events.on("request", (request, event, tags) => {
		if (tags.error) {
			console.log(`Request ${event.request} error: ${event.error ? event.error.message : JSON.stringify(tags)}`);
		}
	});

server.events.on("response", request => {
		console.log(`Response sent: ${request.response}`);
	});

I also have

debug: { request: ["error"] }

set when I create the server.

Basically, to test, I made a curl request to the route and aborted it before 5 seconds.

I expected to get both an "abort" and a "close" tag for my request listener, then expected to get a null response for my "response" listener.

The results are as follows:

request
Debug: request, error, abort
Request <<id>> error: {"request":true,"error":true,"abort":true}
(5 seconds pass)
response
Response sent: [object Object]

As you can see, the response object is actually being set. Previously this was behaving correctly. I was getting an additional "closed" tag.

  • node version: 8.9
  • hapi version: 17.2.0
  • os: Ubuntu Linux 17 (But also on a windows VM on Azure)
  • any other relevant information:
    Using request.response._payload I can see that payload is "null" when I expect it to be.

However, using _payload on Azure Web App does not work correctly (request.response._payload) is always set. I would hazard a guess its due to IIS or a loadbalancer infront of the Web App.

Any suggestions?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 19, 2018

What about this is broken? What's the actual issue here?

@kanongil
Copy link
Contributor

@kanongil kanongil commented Jan 19, 2018

  1. No close / abort events are generated if the request is closed before the handler has finished processing.
  2. request.response is not null on such response events.

@PatrickQuintal
Copy link
Author

@PatrickQuintal PatrickQuintal commented Jan 20, 2018

@hueniverse
Sorry, I should of made it more clear.
Its as @kanongil says. (Thanks)

Additionally, there is no "event.error" (Which, I recall seeing last time)

@hapijs hapijs deleted a comment from bschwartz757 May 21, 2018
@hueniverse hueniverse self-assigned this Aug 1, 2018
@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 1, 2018

The only actual issue is that when a response is not actually transmitted, we still keep it in request.response which is a bug. Once a request has completed, request.response should only reflect what is actually sent back.

However, the previous indicators for aborted requests are going to be a bit different now. Basically, I don't see a way for an incoming request to cause a close stream error that is not also aborted. When a request has been aborted, you are getting the right logs. The additional flag is no longer set because of how the listeners are now implemented. The point is, you don't need it.

@hueniverse hueniverse added the bug label Aug 1, 2018
@hueniverse hueniverse changed the title server.events.on behaving strangely on 17.2.0 request.response should be null when response failed to transmit Aug 1, 2018
@hueniverse hueniverse added this to the 17.5.3 milestone Aug 1, 2018
@hueniverse hueniverse closed this in 939980c Aug 1, 2018
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants