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

fix(gateway): remove buffer-peek-stream #2227

Closed
wants to merge 2 commits into from
Closed

fix(gateway): remove buffer-peek-stream #2227

wants to merge 2 commits into from

Conversation

lidel
Copy link
Member

@lidel lidel commented Jul 8, 2019 •
edited
Loading

This PR fixes "random" daemon crashes with NodeError: Cannot call write after a stream was destroyed (details in libp2p/js-libp2p#374)

It removes buffer-peek-stream which caused uncaught errors inside of Hapijs when HTTP client closed HTTP connection before receiving entire payload from a Gateway port.

After falling into rabit hole of accounting for how Hapijs handles Node streams I decided to take step back, change approach and replace stream peeking with much simpler code that does not introduce intermediate streams.

Steps to reproduce: libp2p/js-libp2p#374 (comment)

Closes libp2p/js-libp2p#374
Closes libp2p/pull-mplex#13

@lidel
fix(gateway): remove buffer-peek-stream
6524f28 
This removes buffer-peek-stream which caused uncaught errors
inside of Hapijs and replaces it with much simpler approach
to set content-type header in Gateway responses.

Closes libp2p/js-libp2p#374
Closes libp2p/pull-mplex#13

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel requested review from alanshaw and jacobheun July 8, 2019 12:53
@lidel
refactor: use rangeResponse flag
01419cb 
License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
alanshaw
alanshaw approved these changes Jul 8, 2019
View reviewed changes
Copy link
Member

@alanshaw alanshaw left a comment

Choose a reason for hiding this comment

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

Nice! Love the alternative approach 👍

@lidel lidel mentioned this pull request Jul 8, 2019
Closed
@jacobheun
Copy link
Contributor

This looks like a good alternative.

Do we have Gateway tests we can add for the early hangups from clients? If we're going to deploy any js-ipfs nodes we should get more tests around abusive client behavior.

@lidel
Copy link
Member Author

lidel commented Jul 8, 2019 •
edited
Loading

@jacobheun not at the moment: gateway tests in js-ipfs skip HTTP transport and inject request directly to hapijs using provided DSL:

js-ipfs/test/gateway/index.js

Lines 342 to 345 in 01419cb

const resFull = await gateway.inject({
method: 'GET',
url: '/ipfs/' + fileCid
})

I agree end-to-end tests like one you described are needed, but I'd argue its true not only for js-ipfs.
We need "gateway interop tests" that ensure both go and js act the same way.

AFAIK we don't have gateway interop test suite, but creating one could be Q3 OKR under "gateway as a product" or web browser maintenance work. Having that kicked off, I'll be happy to seed the interop test suite with tests for issue at hand and other things (I've already started collecting potential issues around HTTP headers in ipfs/in-web-browsers#132)

jacobheun
jacobheun approved these changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

🚀 looks good!

@alanshaw
Copy link
Member

alanshaw commented Jul 10, 2019 •
edited
Loading

Note: @lidel is setting up tests for gateway interop here ipfs/interop#76

@alanshaw
Copy link
Member

alanshaw commented Jul 10, 2019 •
edited
Loading

@lidel is it possible to test this fix using the inject interface?

@lidel
Copy link
Member Author

lidel commented Jul 10, 2019 •
edited
Loading

@alanshaw I tried briefly, but was unable to reproduce it without spawning real server. That is why I'd rather have it in ipfs/interop#76

@alanshaw alanshaw mentioned this pull request Jul 12, 2019
Closed
54 tasks
lidel added a commit that referenced this pull request Jul 12, 2019
@lidel
fix(gateway): disable compression
8869b2d 
This is an alternative to #2227
that does not hit datastore twice.

The underlying error was caused by multiple PassThrough/pipe calls.
Turns out go-ipfs does not compress anything by default, so we can
remove PassThrough responsible for streaming of compressed payload
and fix the issue while keeping optimization introduced by buffer-peek-stream.

Closes libp2p/js-libp2p#374
Closes libp2p/pull-mplex#13

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel mentioned this pull request Jul 12, 2019
Merged
alanshaw pushed a commit that referenced this pull request Jul 12, 2019
@lidel
fix(gateway): disable compression (#2245)
4ee28e0 
This is an alternative to #2227
that does not hit datastore twice.

The underlying error was caused by multiple PassThrough/pipe calls.
Turns out go-ipfs does not compress anything by default, so we can
remove PassThrough responsible for streaming of compressed payload
and fix the issue while keeping optimization introduced by buffer-peek-stream.

Closes libp2p/js-libp2p#374
Closes libp2p/pull-mplex#13

License: MIT
Signed-off-by: Marcin Rataj <lidel@lidel.org>
@lidel lidel deleted the fix/remove-buffer-peek-stream branch July 12, 2019 22:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alanshaw alanshaw alanshaw approved these changes

@hugomrdias hugomrdias hugomrdias approved these changes

@jacobheun jacobheun jacobheun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot call write after a stream was destroyed Cannot call write after a stream was destroyed
4 participants
@lidel @jacobheun @alanshaw @hugomrdias