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

Fix indexer finishing too soon #23

Merged
merged 3 commits into from
Jun 29, 2019
Merged

Fix indexer finishing too soon #23

merged 3 commits into from
Jun 29, 2019

Conversation

00-matt
Copy link
Contributor

@00-matt 00-matt commented Jun 27, 2019

Fixes #18

@00-matt
Copy link
Contributor Author

00-matt commented Jun 27, 2019

Looks like the unit test for the indexer is failing because the result array is now in a different order. Should I rewrite the test to compare the arrays in any order, or does the order matter in any way?

edit Test now passes

@f-prime
Copy link
Owner

f-prime commented Jun 27, 2019

Screen Shot 2019-06-27 at 2 53 17 PM

Just tested this with node-fist and it still seems to not be indexing the pig in a cage phrase. Have you tested it yet or just used the built in test suite?

@f-prime f-prime added this to In progress in Fist Todo Jun 27, 2019
@f-prime f-prime moved this from In progress to Done in Fist Todo Jun 27, 2019
@f-prime f-prime moved this from Done to In progress in Fist Todo Jun 27, 2019
@00-matt
Copy link
Contributor Author

00-matt commented Jun 27, 2019

Have you tested it yet or just used the built in test suite?

Did you bump up READ_MAX in server.c? The input is a little longer than 1024 chars so the test case doesn't fit in the buffer by default.

diff --git a/fist/server.c b/fist/server.c
index bd889f1..43ab062 100644
--- a/fist/server.c
+++ b/fist/server.c
@@ -18,7 +18,7 @@
 
 // TODO: extract to config file?
 #define MAX_PHRASE_LENGTH 10
-#define READ_MAX 1024
+#define READ_MAX 16384
 #define SO_BACKLOG 10
 #define SAVE_SECONDS 120

@f-prime
Copy link
Owner

f-prime commented Jun 28, 2019

I just changed the READ_MAX size and still getting issues. Now it seems to be indexing the last words properly but pig in a cage still does not show up when searched.

Maybe the indexer needs to be rethought out? It seems like there are some fundamental flaws somewhere because the code seems fine.

Screen Shot 2019-06-27 at 11 27 41 PM

@00-matt
Copy link
Contributor Author

00-matt commented Jun 28, 2019

I've rebased to include the fix for longer commands, can you try it now?

@f-prime
Copy link
Owner

f-prime commented Jun 28, 2019

So just tested this and it works, but there is another bug that has come up. There's a malloc error that pops up after indexing a few different text blocks. The most reliable way to cause the error is with the following code:

const { FistConnection } = require("node-fist");
const fist = new FistConnection("localhost", 5575);
for(var i = 0; i < 100; i++) {
    fist.index("song", "fitter, happier more productive comfortable not drinking too much regular exercise at the gym, three days a week getting on better with your associate employee contemporaries at ease eating well, no more microwave dinners and saturated fats a patient, better driver a safer car, baby smiling in back seat sleeping well, no bad dreams no paranoia careful to all animals, never washing spiders down the plughole keep in contact with old friends, enjoy a drink now and then will frequently check credit at moral bank, hole in wall favours for favours, fond but not in love charity standing orders on sundays, ring-road supermarket no killing moths or putting boiling water on the ants car wash, also on sundays no longer afraid of the dark or midday shadows, nothing so ridiculously teenage and desperate nothing so childish at a better pace, slower and more calculated no chance of escape now self-employed concerned, but powerless an empowered and informed member of societ, pragmatism not idealism will not cry in public less chance of illness tires that grip in the wet, shot of baby strapped in backseat a good memory still cries at a good film still kisses with saliva no longer empty and frantic like a cat tied to a stick that's driven into frozen winter shit, the ability to laugh at weakness calm, fitter, healthier and more productive a pig in a cage on antibiotics");
} 

This cases the error below

Screen Shot 2019-06-28 at 8 26 45 AM

I tested this with release version 0.0.1 and the error does not occur.

After looking over the code and doing some testing I found that the error is occurring in server.c when this->last_command is freed here:

https://github.com/00-matt/fist/blob/fix-18/fist/server.c#L250

@00-matt
Copy link
Contributor Author

00-matt commented Jun 28, 2019

It looks like this happens when multiple commands are sent at the same time. If you quickly send commands in quick succession (so that they fit in the same recv() buffer) they get sent to process_command() as one and this makes everything explode.

I think that the best solution would be to split the buffer on \r\n and call process_command() multiple times in a loop, like what the Node.js client library does: https://github.com/00-matt/node-fist/blob/4cd320458a7756a2e4bb7b2e6bc07003c6bd59e4/index.js#L20-L25

@f-prime
Copy link
Owner

f-prime commented Jun 28, 2019

Ah, okay that makes sense. So this a separate issue anyway. This PR seems to fix the original problem. I will merge it if you have no other changes to make, let me know.

@f-prime
Copy link
Owner

f-prime commented Jun 29, 2019

I am going to merge this.

@f-prime f-prime merged commit 629cef3 into f-prime:master Jun 29, 2019
Fist Todo automation moved this from In progress to Done Jun 29, 2019
@00-matt 00-matt deleted the fix-18 branch June 30, 2019 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Fist Todo
  
Done
Development

Successfully merging this pull request may close these issues.

Large blocks of text do not get indexed properly
2 participants