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

rust/projects: fix sled test cli_access_server_sled_engine #279

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frobiac
Copy link

@frobiac frobiac commented Sep 3, 2019

The sled test in cli_access_server fails because the second connection attempt
is made too fast after killing the first server.

Verified with

  • rustc 1.37 as well as current 1.39.0-nightly (9af17757b 2019-09-02)
  • in both Project-3 and 4
  • and the given example solution and my own implementation

System is running Void Linux on a i5-5200U.

thread 'main' panicked at 'should be `able` to open configured file at "/tmp/.tmpX9hOJN/db";
IO error: could not acquire appropriate file lock on "/tmp/.tmpX9hOJN/db":
Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" }',
/home/suka/.cargo/registry/src/github.com-1ecc6299db9ec823/pagecache-0.19.1/src/config.rs:202:13

Inserting a minimal delay of at least 30ms here solves the issues in all the
combinations mentioned above. Other config options in sled itself did not help.

@CLAassistant
Copy link

CLAassistant commented Sep 3, 2019

CLA assistant check
All committers have signed the CLA.

The sled test in cli_access_server fails because the second connection attempt
is made too fast after killing the first server.

Verified with
  - rustc 1.37 as well as current 1.39.0-nightly (9af17757b 2019-09-02)
  - in both Project-3 and 4
  - and the given example solution and my own implementation

System is running Void Linux on a i5-5200U.

```
thread 'main' panicked at 'should be `able` to open configured file at "/tmp/.tmpX9hOJN/db";
IO error: could not acquire appropriate file lock on "/tmp/.tmpX9hOJN/db":
Os { code: 11, kind: WouldBlock, message: "Resource temporarily unavailable" }',
/home/suka/.cargo/registry/src/github.com-1ecc6299db9ec823/pagecache-0.19.1/src/config.rs:202:13
```

Inserting a minimal delay of at least 30ms here solves the issues in all the
combinations mentioned above. Other config options in sled itself did not help.
@zz-jason zz-jason changed the title Project-3 & 4: Fix sled test cli_access_server_sled_engine rust/projects: fix sled test cli_access_server_sled_engine Feb 29, 2020
@brson
Copy link
Contributor

brson commented Mar 4, 2020

Thanks for the patch and sorry for not reviewing earlier @frobiac.

I'd rather not add sleeps to fix broken tests. The correct way to fix this is to hook into some event before continuing the test. If that's not feasible then let's just #[ignore] the test with a comment about why.

Copy link
Contributor

@brson brson left a comment

Choose a reason for hiding this comment

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

Per previous comment.

@morenol
Copy link

morenol commented Apr 18, 2020

Hi @brson, I was able to solve this problem by installing sled in the dev-dependencies section as:

sled = { version = "0.31.0", features = ["testing"] }

Does this make sense?

On the other hand, I don't get that error here in project-3 but I get that in mine.

@brson
Copy link
Contributor

brson commented Apr 28, 2020

@morenol thanks for the suggestion.

On closer inspection, I see the tests are already filled with sleeps :( This is not a practice I want to endorse in this course!

Offhand I don't see why putting sled = { version = "0.31.0", features = ["testing"] } in dev-dependencies would lead to a change in behavior. Sled's "testing" feature looks to be for its own testing purposes.

That said, simply upgrading sled would be a good idea. I'm sure it's had a lot of bug fixes, and I'd be happy to take a patch that does the upgrade.

When I look at the test that is failing, what I suspect is happening is that the tests are not properly waiting for the first server process to exit. The kill docs don't say they wait for exit, just that they send the kill signal.

So I think the tests should probably be changed such that every kill is followed by a wait.

Do you mind trying that and seeing if it fixes this bug?

@morenol
Copy link

morenol commented Apr 28, 2020

@brson Yes, I think that I could do that.

I made that because I saw that the error can be avoided here in the sled library. But I guess that the kill and wait should be better.

Update: I tried the kill and wait, and the tests are failing (and sometimes they are passing)

@Edison-A-N
Copy link

I meet the same issue, are there any update?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants