fix: add `disconnect` to memcache driver #865

Julien-R44 · 2023-07-05T22:01:46Z

Please check if the PR fulfills these requirements

Followed the Contributing guidelines.
Tests for the changes have been added (for bug fixes/features) with 100% code coverage.
Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Without this change, this code was never exiting :

import Keyv from 'keyv';
import KeyvMemcache from './src/index';

let uri = 'localhost:11211';
const keyvMemcache = new KeyvMemcache(uri);
const keyv = new Keyv({store: keyvMemcache});

keyv.get('foo').then(() => {
  keyv.disconnect()
})

So just added a disconnect method to the memcache driver, and now works fine

Wanted to add a test for the same :

test.serial('close connection successfully', async t => {
	const keyv = new Keyv({store: keyvMemcache});
	t.is(await keyv.get('foo'), undefined);
	await keyv.disconnect();
	try {
		await keyv.get('foo', 34);
		t.fail();
	} catch {
		t.pass();
	}
});

but it doesn't work. memjs throws 0 errors when you do a .get or .set when the connection is closed. So not sure how to handle this

codecov · 2023-07-06T11:35:50Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (05a0e89) 100.00% compared to head (6cfe2ad) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #865   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           22        22           
  Lines         2590      2594    +4     
  Branches       339       339           
=========================================
+ Hits          2590      2594    +4

Impacted Files	Coverage Δ
packages/memcache/src/index.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

jaredwray · 2023-07-06T12:11:15Z

Hi @Julien-R44 👋 - Thanks so much for sending the pull request. If you could add in the unit tests for this change then we could get this merged.

Julien-R44 · 2023-07-06T14:35:11Z

Added a test for this one.
Also opened an issue on memjs to know if this behavior is by design or just a bug : memcachier/memjs#172

If this is by design, we should absolutely fix it on the keyv side. Look at this code :

const memcached = new KeyvMemcache('localhost:11211')
const memcache = new Keyv({ store: memcached })

try {
  console.log('go')
  await memcache.get('foo')
  await memcache.disconnect()
  await memcached.get('foo')
  console.log('done 1')
} catch (err) {
  console.log('done 2')
}

console.log('done 3')

Guess the output 😄
Output will be :

go
disconnect

output will not include done 1, done 2, or done 3

Because this promise : https://github.com/JuliensForks/keyv/blob/main/packages/memcache/src/index.ts#L48 is never resolving.
So it's quite a big problem.

( This will only be the case after this PR has been applied. Because right now, the disconnect() on the memcache driver does nothing. )

jaredwray · 2023-07-06T18:03:13Z

packages/memcache/src/index.ts

@@ -163,6 +163,10 @@ class KeyvMemcache<Value = any> extends EventEmitter implements Store<Value> {
 			});
 		});
 	}
+
+	async disconnect(): Promise<void> {
+		this.client.quit();


to force the quit you might want to do close() as that might resolve the issue you are seeing

Just tried, and it changes nothing. By reading comments in memjs, i believe it's better to use quit as it is a clean exit

ok. thanks for trying. Should we wait to see what memjs says?

jaredwray · 2023-07-06T18:05:31Z

Added a test for this one. Also opened an issue on memjs to know if this behavior is by design or just a bug : memcachier/memjs#172

If this is by design, we should absolutely fix it on the keyv side. Look at this code :
const memcached = new KeyvMemcache('localhost:11211')
const memcache = new Keyv({ store: memcached })

try {
  console.log('go')
  await memcache.get('foo')
  await memcache.disconnect()
  await memcached.get('foo')
  console.log('done 1')
} catch (err) {
  console.log('done 2')
}

console.log('done 3')
Guess the output 😄 Output will be :
go
disconnect
output will not include done 1, done 2, or done 3

Because this promise : https://github.com/JuliensForks/keyv/blob/main/packages/memcache/src/index.ts#L48 is never resolving. So it's quite a big problem.

( This will only be the case after this PR has been applied. Because right now, the disconnect() on the memcache driver does nothing. )

that is interesting. Did you want to provide a fix for this in the library as i would like to not add it if it does nothing at this time. you could also look at close which might do the trick.

Julien-R44 · 2023-07-06T18:20:00Z

Not sure I understand what you're proposing
I tried quit(), but it doesnt changes anything
I'll recap to make sure we understand each other :

This PR adds a disconnect() method, which works fine. It really disconnects us from memcache server.
On the other hand, introducing this method causes the bug I mentioned just above, which could probably annoy some people using keyv.

For this, we have two solutions:

Either we wait for a response from the Memjs maintainers, to find out whether this is a bug or normal behavior. Then we act accordingly. I don't think we'll get an answer for a while, as memjs doesn't seem to be extremely maintained.
Or we could add a fix on the keyv side. The fix on the keyv side would be more or less the same as what I added in the tests. So basically a timeout system. if there's no response after X seconds, then we resolve the various promises with an error.
This way, the program can continue to run.

let me know what you think is best and i'll do a PR

jaredwray · 2023-07-06T18:23:23Z

Not sure I understand what you're proposing I tried quit(), but it doesnt changes anything I'll recap to make sure we understand each other :

This PR adds a disconnect() method, which works fine. It really disconnects us from memcache server.

On the other hand, introducing this method causes the bug I mentioned just above, which could probably annoy some people using keyv.

For this, we have two solutions:

Either we wait for a response from the Memjs maintainers, to find out whether this is a bug or normal behavior. Then we act accordingly. I don't think we'll get an answer for a while, as memjs doesn't seem to be extremely maintained.

Or we could add a fix on the keyv side. The fix on the keyv side would be more or less the same as what I added in the tests. So basically a timeout system. if there's no response after X seconds, then we resolve the various promises with an error.
This way, the program can continue to run.

let me know what you think is best and i'll do a PR

If you want to do the fix we can support that. If not, lets wait a week to see what they say.

jaredwray · 2023-07-12T20:17:42Z

@Julien-R44 looks like there was a comment. memcachier/memjs#172 (comment)

Thoughts on that?

Julien-R44 · 2023-07-12T20:38:32Z

Yeah, I'll try to send a PR to memjs and then update our PR accordingly once the problem has been fixed
Thanks

jaredwray · 2023-07-26T05:30:35Z

Yeah, I'll try to send a PR to memjs and then update our PR accordingly once the problem has been fixed Thanks

Any word on the PR for memjs? Anything I can do to help?

Julien-R44 · 2023-07-26T19:25:50Z

hey! i am sorry to leaving this hanging. i started looking but it's a bit complicated. it's very oldschool js code and a bit low level.
i'll have to get back into it when i have a bit more time.
but it's still relevant. we're probably planning to use keyv for the caching module in the next version of AdonisJS

Julien-R44 · 2023-07-31T16:43:37Z

sorry, I decided to not add memcached support to my application after all. So unfortunately I won't be able to complete this PR. I'll let you decide whether to close it or not

jaredwray · 2023-07-31T16:46:28Z

sorry, I decided to not add memcached support to my application after all. So unfortunately I won't be able to complete this PR. I'll let you decide whether to close it or not

@Julien-R44 - thanks for the update and we will look to see if we want to continue on this PR.

jaredwray · 2023-08-06T13:33:51Z

closing and will look at this later

fix: add disconnect to memcache driver

a2d8d89

test: add test for memcache driver disconnect method

f316bac

Julien-R44 added 2 commits July 6, 2023 18:37

chore: complete comment

c0d8a4d

style: fix lint

6cfe2ad

jaredwray reviewed Jul 6, 2023

View reviewed changes

jaredwray closed this Aug 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add `disconnect` to memcache driver #865

fix: add `disconnect` to memcache driver #865

Julien-R44 commented Jul 5, 2023

codecov bot commented Jul 6, 2023 •

edited

Loading

jaredwray commented Jul 6, 2023

Julien-R44 commented Jul 6, 2023 •

edited

Loading

jaredwray Jul 6, 2023

Julien-R44 Jul 6, 2023

jaredwray Jul 6, 2023

jaredwray commented Jul 6, 2023

Julien-R44 commented Jul 6, 2023 •

edited

Loading

jaredwray commented Jul 6, 2023

jaredwray commented Jul 12, 2023

Julien-R44 commented Jul 12, 2023

jaredwray commented Jul 26, 2023

Julien-R44 commented Jul 26, 2023

Julien-R44 commented Jul 31, 2023

jaredwray commented Jul 31, 2023

jaredwray commented Aug 6, 2023

fix: add disconnect to memcache driver #865

fix: add disconnect to memcache driver #865

Conversation

Julien-R44 commented Jul 5, 2023

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

jaredwray commented Jul 6, 2023

Julien-R44 commented Jul 6, 2023 • edited Loading

jaredwray Jul 6, 2023

Choose a reason for hiding this comment

Julien-R44 Jul 6, 2023

Choose a reason for hiding this comment

jaredwray Jul 6, 2023

Choose a reason for hiding this comment

jaredwray commented Jul 6, 2023

Julien-R44 commented Jul 6, 2023 • edited Loading

jaredwray commented Jul 6, 2023

jaredwray commented Jul 12, 2023

Julien-R44 commented Jul 12, 2023

jaredwray commented Jul 26, 2023

Julien-R44 commented Jul 26, 2023

Julien-R44 commented Jul 31, 2023

jaredwray commented Jul 31, 2023

jaredwray commented Aug 6, 2023

fix: add `disconnect` to memcache driver #865

fix: add `disconnect` to memcache driver #865

codecov bot commented Jul 6, 2023 •

edited

Loading

Julien-R44 commented Jul 6, 2023 •

edited

Loading

Julien-R44 commented Jul 6, 2023 •

edited

Loading