Skip to content
This repository has been archived by the owner on Jan 16, 2024. It is now read-only.

HTTP2Stream request handling appears to cut off exception messages #67

Closed
jshslt opened this issue Aug 1, 2017 · 4 comments
Closed
Assignees

Comments

@jshslt
Copy link

jshslt commented Aug 1, 2017

Hi,
We noticed that when we get a MessageRequest::onExceptionReceived callback, the exception message tends to look cut off. since this is often (always?) JSON, it leads to improperly formatted content.

It looks like this is due to how HTTP2Stream::m_exceptionBeingProcessed is used; as soon as it's non-zero length, HTTP2Stream::notifyRequestObserver assumes the message is complete and sends it to the request callback.

@scotthea-amazon
Copy link
Contributor

Hi Jshslt,
Thank you for bringing this to our attention!

We can see some obvious code paths that could lead to this and will be working on a fix for those. To make sure we fully address the issue you described, can you provide us with a test case that triggers the error for you?

Thank you!
-SWH

@jshslt
Copy link
Author

jshslt commented Aug 3, 2017

@scotthea-amazon sorry, I don't have an explicit repro case for this. Just a guess but it might trigger if you try and send an AVS event immediately after (e.g.) pulling a network cable, but before the SDK is aware of the disconnect.

if it helps, here's an example of a truncated exception message we got -

{"header":{"namespace":"System","name":"Exception","messageId":"1af199cd-b665-4d45-95b8-341884959e11"},"payload":{"code":"INVALID_R

@dhpp
Copy link
Contributor

dhpp commented Aug 15, 2017

Hi jshslt,

I wanted to jump into this discussion. Looking over the HTTP2Transport and HTTP2Stream code within ACL, you've brought up an interesting use case.

The place where this happens in our code is HTTP2Transport::cleanupFinishedStreams, where the code checks for CURLMSG_DONE, then invokes the notifyRequestObserver overload on the stream which will send out the exception message. What is interesting to me is that the use case you've brought up is essentially a broken network, where partial data will be correctly received, but then libcurl will internally time out and close the stream.

Could I suggest you edit the final else within cleanupFinishedStreams:

auto it = m_activeStreams.find(message->easy_handle);
if (it != m_activeStreams.end()) {

long responseCode = 0;
curl_easy_getinfo(it->first, CURLINFO_RESPONSE_CODE, &responseCode);
ACSDK_INFO(LX("stream response code").m(std::to_string(responseCode)));

it->second->notifyRequestObserver();
cleanupStream(it->second);
} else {
ACSDK_ERROR(LX("cleanupFinishedStreamError").d("reason", "streamNotFound"));
}

I would like to know what the reponseCode is for the cases where received exception messages are incomplete. If it's something that libcurl is correctly exposing to us, we can then update that code accordingly to call notifyRequestObserver(MessageRequest::Status::INTERNAL_ERROR) instead.

Thanks.

frozenberg added a commit that referenced this issue Oct 2, 2017
- Changes in this update:

	- Better GStreamer error reporting. MediaPlayer used to only report   `MEDIA_ERROR_UNKNOWN`, now reports more specific errors as defined in `ErrorType.h`.
	- Codebase has been formatted for easier reading.
	- `DirectiveRouter::removeDirectiveHandler()` signature changed and now returns a bool indicating if given handler should be successfully removed or not.
	- Cleanup of raw and shared pointers in the creation of `Transport` objects.
	- `HTTP2Stream`s now have IDs assigned as they are acquired as opposed to created, making associated logs easier to interpret.
	- `AlertsCapabilityAgent` has been refactored.
		- Alert management has been factored out into an `AlertScheduler` class.
	- Creation of Reminder (implements Alert) class.
	- Added new capability agent for `PlaybackController` with unit tests.
	- Added Settings interface with unit tests.
	- Return type of `getOffsetInMilliseconds()` changed from `int64_t` to `std::chronology::milliseconds`.
	- Added `AudioPlayer` unit tests.
	- Added teardown for all Integration tests except Alerts.
	- Implemented PlaylistParser.

- Bug fixes:

	- AIP getting stuck in `LISTENING` or `THINKING` and refusing user input on network outage.
	- SampleApp crashing if running for 5 minutes after network disconnect.
	- Issue where on repeated user barge-ins, `AudioPlayer` would not pause. Specifically, the third attempt to “Play iHeartRadio” would not result in currently-playing music pausing.
	- Utterances being ignored after particularly long TTS.
	- GStreamer errors cropping up on SampleApp exit as a result of accessing the pipeline before it’s been setup.
	- Crashing when playing one URL after another.
	- Buffer overrun in Alerts Renderer.
	- [SampleApp crashing when issuing "Alexa skip" command with iHeartRadio.](#153)
	- [`HTTP2Transport` network thread triggering a join on itself.](#127)
	- [`HTTP2Stream` request handling truncating exception messages.](#67)
	- [`AudioPlayer` was attempting an incorrect state transition from `STOPPED` to `PLAYING` through a `playbackResumed`.](#138)
@kclchan
Copy link
Contributor

kclchan commented Oct 3, 2017

The issue is fixed in v1.1.0. Thanks!

@kclchan kclchan closed this as completed Oct 3, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants