WIP: Add a concurrent test for issue creation

[typeless]

Related to #7887, #7944

[guillep2k]

Can you rebase your changes on my PR? Something like:

git fetch origin pull/7944/head:a-branch-name-of-your-liking

This way Gitea's drone can test it with the code that in theory should not fail.

[guillep2k]

You are stressing the database here (what you need) but also stressing other layers, like the http client & server, the computer resources, etc. I think it would be preferrable if you'd call the models/issue.go methods instead.

[typeless]

Agreed. I'll try to figure out how to do that.

[guillep2k]

I think a great deal of times this function will fail for reasons unrelated to a duplicate key (which is what this test is for) and give false negatives. It's important to check the error and verify it is actually a duplicate key; other errors should be ignored. Gitea will be built in all kinds of environments, e.g. small Raspberry Pi boards.
Also, this might extend the test times beyond the alloted values in the drone (something to be dealt with).

[typeless]

That's true. But I am afraid that there are some other undiscovered bugs, so an integration test might be helpful to an extent if it can reveal them.

[guillep2k]

Note: launching 1,000 threads will not result in 1,000 simultaneous calls to the database, as the database can have a reduced connection pool. I believe that there are defaults in golang for the number of simultaneous threads too.

[typeless]

That makes sense. Noted.
I thought that it could minimize the probability of false negative.
I'll revisit my presumptions considering there is a limited number of connections.

[sapk]

Good idea for PR. I would suggest to reduce the number of call for the integration test and do more intensive test in a unit tests.

[sapk]

@guillep2k have you try to cherry pick this PR over your PR to validate ?

[guillep2k]

Have you had any progress with this? Are you still getting random errors from the databases?

I'm thinking that rather than starting with an empty table, this test would be much more efficient if issue is pre-loaded with a large set of rows, like directly inserting 10,000 issues with one task (setting Index manually, no conflicts there) and then launch 1,000 threads to attempt to break the logic.

[typeless]

@guillep2k @sapk
I am still not sure if the code is bug-free. I tested it with the master, and it would hang occasionally.
I tested with #7944. There seemed to be the internal 500 error from POST as well.

I am also considering if the parallel subtests of the testing package would be a better alternative for such kind of tests. I am not familiar with it. what do you think?

[guillep2k]

@guillep2k @sapk
I am still not sure if the code is bug-free. I tested it with the master, and it would hang occasionally.
I tested with #7944. There seemed to be the internal 500 error from POST as well.

I am also considering if the parallel subtests of the testing package would be a better alternative for such kind of tests. I am not familiar with it. what do you think?

I think it's important to bypass the HTTP API for this. Otherwise the chain is too long and any one link can fail, not only the potential dup key problem. Also, it's difficult to stress a database unless you're dealing with it in the most direct way possible.

Having that level of concurrency is unrealistic. There will never be 1,000 threads running at the same time. The database connections are pooled and therefore most clients will be waiting in queue, which does not serve the purpose.

In your tests you're hitting:

goroutine 0 [idle]:
runtime.epollwait(0x4, 0x7ffc92970a00, 0xffffffff00000080, 0x0, 0xffffffff005ddaee, 0x10800000000, 0x0, 0x0, 0x0, 0x0, ...)
/usr/local/go/src/runtime/sys_linux_amd64.s:675 +0x20
runtime.netpoll(0xc000060501, 0xc000054a01)
/usr/local/go/src/runtime/netpoll_epoll.go:71 +0x126
runtime.findrunnable(0xc000050000, 0x0)
/usr/local/go/src/runtime/proc.go:2380 +0x51f
runtime.schedule()
/usr/local/go/src/runtime/proc.go:2525 +0x21c
runtime.goexit0(0xc0011aa600)
/usr/local/go/src/runtime/proc.go:2722 +0x1dd
runtime.mcall(0x15ebb90)
/usr/local/go/src/runtime/asm_amd64.s:299 +0x5b

I'm no golang expert, but I do have experience with this kind of scenario. netpoll_epoll.go sounds like a network wait loop, most probably related with your HTTP request than the database.

I'd restructure this test in the following way:

• Move the test to models/. It will be tested with all databases anyway.
• No HTTP: only database queries through x.Insert() (x is the xorm Engine that will become available inside the models pkg).
• Within the main thread, create a large number of issues (1,000? 10,000? it's worth some tests) by manually assigning the Index value. The goal is to have MAX(Index) do some work.
• Set up a reference timer (e.g. time.Now())
• Set up a var has_errors error with a nil value.
• Create only 4 or 8 threads for inserting. Each thread would loop through this:
  • Insert an issue through models.newIssue(). If errors, set has_errors and return.
  • If X time has passed counting from the set reference (20 seconds? 60 seconds?), return.
  • If has_errors is not nil, return.
  • Repeat.
• In the main thread, just wait for the other threads to finish and then check for has_errors. You can assert that it's nil.

With 4 or 8 threads, chances of simultaneous insert are probably the same as with 1,000, but it's less likely to fail for other causes. At least that's what I think.

[guillep2k]

OK, I couldn't help myself. Please check #7959.

[typeless]

In favor of #7959