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

coreapi not respecting context timeouts #9532

Closed
3 tasks done
philwinder opened this issue Jan 10, 2023 · 3 comments
Closed
3 tasks done

coreapi not respecting context timeouts #9532

philwinder opened this issue Jan 10, 2023 · 3 comments
Assignees
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up

Comments

@philwinder
Copy link

philwinder commented Jan 10, 2023

Checklist

Installation method

built from source

Version

v0.17.0

Config

No response

Description

When using the IPFS HTTP RPC (/api/v0) client, we set timeouts in the context in the normal go way. But when using the coreapi it doesn’t look like timeouts are respected in the same way. For example, when you do a stat with the coreapi, (I belieive) it goes directly to the filesystem to do that stat, therefore there’s no delay and no timeout applied. Is that correct?

In that case, I guess there should still be a timeout somewhere because even when you’re using coreapi it might just take too long to get an object over IPFS. So the question is, where should that timeout now reside?

Context: this test is failing because no timeout is evaluated (and therefore it doesn’t timeout). Previously, when using the HTTP client, the stat would timeout when you set the timeout to 0.

cc: @Jorropo

@philwinder philwinder added kind/bug A bug in existing code (including security flaws) need/triage Needs initial labeling and prioritization labels Jan 10, 2023
@lidel lidel added need/analysis Needs further analysis before proceeding P2 Medium: Good to have, but can wait until someone steps up effort/days Estimated to take multiple days, but less than a week exp/intermediate Prior experience is likely helpful and removed need/triage Needs initial labeling and prioritization labels Jan 19, 2023
@Jorropo Jorropo moved this to 🥞 Todo in IPFS Shipyard Team Jan 19, 2023
@BigLep
Copy link
Contributor

BigLep commented Jan 24, 2023

@Jorropo : what are the next steps on this one?

@Jorropo
Copy link
Contributor

Jorropo commented Jan 25, 2023

I have looked this up, the context is correctly plumbed all the way through and flatfs is the culprit: https://github.com/ipfs/go-ds-flatfs/blob/3b1c91bc3097ec7702347d8a419269ce88e450b8/flatfs.go#L651-L667

The impact is that if you already have the blocks locally and you are using flatfs to store them, we do not check the context and try to run requests until completion.

However things where we really care about context is for high latency operations (bitswap, remote datastore, ...) all of thoses work fine (actually I didn't checked the S3 code).

Given that there is no support for context.Context in golang's std os package (what we use to read and write to files).
I think this is a WONTFIX because most workarounds are costy (at least until golang/go#57928 is not fixed).

There is also a world where we do the dirty:

select {
case <-ctx.Done():
  return nil, ctx.Error()
default:
}

In flatfs operations, but I don't really like that either. That only solve the issue if you happen to call with an already terminated context, if the context would expire while the read or write is happening (and not before), then it does nothing.


More read about the subject: golang/go#20280

@Jorropo Jorropo removed the need/analysis Needs further analysis before proceeding label Jan 25, 2023
@Jorropo Jorropo moved this from 🥞 Todo to 🛑 Blocked in IPFS Shipyard Team Jan 25, 2023
@Jorropo Jorropo moved this from 🛑 Blocked to 🔎 In Review in IPFS Shipyard Team Jan 25, 2023
@Jorropo Jorropo moved this from 🔎 In Review to 🛑 Blocked in IPFS Shipyard Team Jan 25, 2023
@Jorropo Jorropo added effort/hours Estimated to take one or several hours and removed effort/days Estimated to take multiple days, but less than a week labels Jan 25, 2023
@BigLep
Copy link
Contributor

BigLep commented May 11, 2023

Closing given @Jorropo comments in #9532 (comment)

@BigLep BigLep closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
@github-project-automation github-project-automation bot moved this from 🛑 Blocked to 🎉 Done in IPFS Shipyard Team May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/hours Estimated to take one or several hours exp/intermediate Prior experience is likely helpful kind/bug A bug in existing code (including security flaws) P2 Medium: Good to have, but can wait until someone steps up
Projects
No open projects
Archived in project
Development

No branches or pull requests

4 participants